Unlock before cleanup and calling out to subclass hooks for animated images.

[maicki]

• Unlock before cleanup
• Unlock before calling out to animatedImageSet:previousAnimatedImage: to prevent a deadlock.

[garrettmoon]

Is this actually safe? We don't necessarily know what the calling scope is doing and therefore unlocking here could be dangerous? I know it's very unlikely (I can only think of convoluted examples)…

I'm not sure of a better solution though… If we do end up deciding this, we should at least add a comment that unlocking a lock someone else locked might cause us a bad time in the future.

[nguyenhuy]

Agree. Another thing is that with a recursive lock, the lock may have been acquired multiple times and so unlocking once here is not enought.

Ideally, we have to make this method _u_ but it can be painful and may not be possible.

Another idea is dispatching before calling -animatedImageSet:previousAnimatedImage: and deallocating the previous image.

[nguyenhuy]

Shouldn't we wrap the above lines in a shouldCleanup block?

[nguyenhuy]

Hm, can you explain why -setCoverImage: needs to be called unlocked?

Somewhat unrelated: that method acquires the lock right away, so shouldn't we just call -_locked_setCoverImage: directly here?

[nguyenhuy]

Overall, I think it's great to remove this caveat.

[nguyenhuy]

Agree. Another thing is that with a recursive lock, the lock may have been acquired multiple times and so unlocking once here is not enought.

Ideally, we have to make this method _u_ but it can be painful and may not be possible.

Another idea is dispatching before calling -animatedImageSet:previousAnimatedImage: and deallocating the previous image.

[nguyenhuy]

Opps, didn't mean to approve.

[maicki]

Unfortunately this is all not so trivial to solve. If we would like to solve this without dispatching to the next runloop it will take a lot of effort to refactor every (AS) image node and the fact we have to call a delegate subclass hook needs to be considered every time we change the code.

After looking into it more deeply I think there are a couple of ways we could approach this:

1. Refactor ASImageNode / ASNetworkImageNode to add some way to call the subclass hook without the lock held. As mentioned above this would need some massive refactor and also always some knowledge needed so calling the subclass hook will be called eventually.
2. Create an internal serial queue that would call to the subclass method, like a delegate (or move the method out as an internal delegate method in the first place and dispatch the call to the queue)
3. Dispatch the subclass call to the main queue / thread (this would go against the current trend though as it seems we are trying to get rid of calling delegate / subclass hooks on main)
4. Dispatch the call to the current queue (dispatch_get_current_queue). dispatch_get_current_queue is deprecated at the moment and there are a lot of online material that definetly speak against it.
5. Use performSelector:afterDelay:... This would be one way, but as we cannot pass two objects in this methods, we have to create a helper method. This could be factored out into an NSObject category though to reusable schedule a block via performSelector:
6. Use CFRunLoopPerformBlock (There would also be -[NSRunLoop performBlock:] but that's iOS 10+). This would work too to schedule a block on the next run loop cycle and this without having to introduce any more infrastructure around it as we can scheduel a block.

I eventually went with version 6 for now you can see the history too for also implementing 5. Happy for any thoughts or better ideas cc @nguyenhuy @garrettmoon @appleguy @Adlai-Holler

[nguyenhuy]

I think we need to deallocate the previous image after calling out to subclass, otherwise it will not be deallocated in background if the deallocation occurs before the next run loop.

[maicki]

@nguyenhuy You wanna give that another look please