Your changes seems break the Hack I used, which solve the rare case in #3477

Why that SetCompleted exists because , SDWebImageDownloader need to Lock the DownloadOperation, it's a Lock across two class

Because, some rare case, when during URLSessionTask callback, new sd_setImage(with:) loading pipeline begines and hit SDWebImageDownloader loadImageWithURL:, we must ensure All input pipeline completion block is always called.

So, that addNewHandler logic is only useful when Download Operation not into the URLSessionTask:didComplete, because there are a @synchronized(self) call, so once we enter the URLSessionTask:didComplete, new Handlers no longer can added, because it's lock guard outside

So this Lock is here to ensure that behavior. However, seems your changes does not gurantee this behavior ever. Because Downloader no longer notice Whether this DownloadOperation is still can be re-useble

Another issue why I use an associated object, because you forget one feature, Custom Download Operation Class🤮

Not all users use SDWebImageDownloadOperation, they can use their own class. Until v6.0.0 (in v6.0.0 I'll remove this class and ugly design)

This ugly design is used to support some advanced user who Don't use URLSession at all, they write their network stack from scratch like UNIX TCP, like FaceBook or ByteDance, but this can be done via custom SDImageLoader protocol new feature introduced in v5.0, and they can implements the Reusing network task by their own, not SD's duty

Oh, after I read the code agin, seems you use the recursive check ? Check whether there are new handlers (re-using the same Download Operation) during decoding ?

Is there still any possible chance (in theory) can cause some handler called from SDWebImageDownloader not been callback ?

Unfortunately I really didn't give enough thought to why you would want to use an associated object rather than a property😭.
Actually, what I'd like to do is to be able to decode the snapshots of callbackTokens after they've been decoded, and to decode the new tokens that were added during this operation as well

When I used version 5.14.3 in my project again, I caught a rare case where some Views did not receive the completeBlock callback. The reason is that During the URLSessionTask callback, new sd_setImage(with:) loading pipeline starts running with SDWebImageDownloader loadImageWithURL.
My workaround was to recursively check for new tokens after SDWebImageDownloadOperation had finished decoding the snapshot of the callbackTokens, and that seemed to work fine!

Oh, after I read the code agin, seems you use the recursive check ? Check whether there are new handlers (re-using the same Download Operation) during decoding ?

Is there still any possible chance (in theory) can cause some handler called from SDWebImageDownloader not been callback ?

If there is a newly added token before checkDone starts executing and Done executes, it may not be possible to respond to this newly added token.
I can't think of a possible scenario (in theory) where this would be the case🤖

Can we use SD_LOCK or @syncrhonized (self) in the whole checkDone method ? Means, it's a atomic check, no longer allows something that add new handlers during checkDone.

This checkDone does not take much CPU time, because just NSArray compare with pointer (That removeObjectIdenticalTo) (I don't thing one URL can contains massive completionBlock callback...)

Others logic seems reasonable for me

Can we use SD_LOCK or @syncrhonized (self) in the whole checkDone method ? Means, it's a atomic check, no longer allows something that add new handlers during checkDone.

This checkDone does not take much CPU time, because just NSArray compare with pointer (That removeObjectIdenticalTo) (I don't thing one URL can contains massive completionBlock callback...)

Others logic seems reasonable for me

I envisioned a rare case.
If a new request in SDWebImageDownloader determines if operation can be reused, and at that point operation.isFinished == false, then he will try to call addHandler to reuse the operation. The addHandler method will be locked by @synchronized(operation). But if operation is executing the atomic checkDone method in another thread in the meantime, then he will add a new token to operation.callbackTokens after executing the whole [self done] method, and the token will not be available for callback.😵
Is this possible❓
If so, wouldn't we need to make sure that the process in SDWebImageDownloader that determines whether operation is reusable, and the process in operation.checkDone are mutually exclusive in order for this not to happen.

Can we use SD_LOCK or @syncrhonized (self) in the whole checkDone method ? Means, it's a atomic check, no longer allows something that add new handlers during checkDone.
This checkDone does not take much CPU time, because just NSArray compare with pointer (That removeObjectIdenticalTo) (I don't thing one URL can contains massive completionBlock callback...)
Others logic seems reasonable for me

I envisioned a rare case. If a new request in SDWebImageDownloader determines if operation can be reused, and at that point operation.isFinished == false, then he will try to call addHandler to reuse the operation. The addHandler method will be locked by @synchronized(operation). But if operation is executing the atomic checkDone method in another thread in the meantime, then he will add a new token to operation.callbackTokens after executing the whole [self done] method, and the token will not be available for callback.😵 Is this possible❓ If so, wouldn't we need to make sure that the process in SDWebImageDownloader that determines whether operation is reusable, and the process in operation.checkDone are mutually exclusive in order for this not to happen.

That why my SDWebImageDownloaderOperationSet/GetOperation usede for.

It's a atomic operation need two classes to operate. Which means, Downloader lock the operation.

Use @synrhonized (operation) in Downloader, and @synchronized (self) in Download operation.

Just do so. In SDWebImage v6.0.0,, this ugly design and Download operation class will be removed.

But if operation is executing the atomic checkDone method in another thread in the meantime, then he will add a new token to operation.callbackTokens after executing the whole [self done] method

No, the [self checkDone] and [self done] two, are also gurard by @synchronized (self). Which is the lock

So, any logic in downloader (check whether to re-use or create new downloader operation) must execute after the [self done] and the operation.isFinished = YES, so it will never bind new handlers to a finished one.

But if operation is executing the atomic checkDone method in another thread in the meantime, then he will add a new token to operation.callbackTokens after executing the whole [self done] method

No, the [self checkDone] and [self done] two, are also gurard by @synchronized (self). Which is the lock

So, any logic in downloader (check whether to re-use or create new downloader operation) must execute after the [self done] and the operation.isFinished = YES, so it will never bind new handlers to a finished one.

Oh, I made a mistake.
When I was looking back at the code in SDWebImageDownloader that checks for reusable operations, I was looking at version 5.14.3 of SDWebImage from another project I'm working on. The newer version also uses @syncrhonised (self) locking on the part that checks for operation.isFinished. This will not cause the scenario I envisioned.

But if operation is executing the atomic checkDone method in another thread in the meantime, then he will add a new token to operation.callbackTokens after executing the whole [self done] method

No, the [self checkDone] and [self done] two, are also gurard by @synchronized (self). Which is the lock
So, any logic in downloader (check whether to re-use or create new downloader operation) must execute after the [self done] and the operation.isFinished = YES, so it will never bind new handlers to a finished one.

Oh, I made a mistake. When I was looking back at the code in SDWebImageDownloader that checks for reusable operations, I was looking at version 5.14.3 of SDWebImage from another project I'm working on. The newer version also uses @syncrhonised (self) locking on the part that checks for operation.isFinished. This will not cause the scenario I envisioned.

If you have a fork use another lock not @synchronized, you need the downloader and downloader operation Hold the same lock. For example, dispatch_semaphore. You need a public property of download operation and expose to downloader.

Just the same, these two classes Need the same lock, no matter whatever type lock is.

Back to this PR, I think it's just OK to mark the whole checkDown with the @synchonized (self), so it in theory can never cause any callback missing.

PS: This bug (missing callback handler) exists on Kingfisher as well, because it don't use lock, but no body cares.. (only me get some blames...)

I have tried to run the modified code in my local demo and in another project and it seems to work. I'd appreciate it if you could review it for me when you have time☕️.

LGTM