Skip to content

Conversation

@dreampiggy
Copy link
Contributor

@dreampiggy dreampiggy commented Nov 3, 2017

Add a more common macro to do current queue check

New Pull Request Checklist

  • I have read and understood the CONTRIBUTING guide

  • I have read the Documentation

  • I have searched for a similar pull request in the project and found none

  • I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)

  • I have added the required tests to prove the fix/feature I am adding

  • I have updated the documentation (if necessary)

  • I have run the tests and they pass

  • I have run the lint and it passes (pod lib lint)

This merge request fixes / reffers to the following issues: #2092

Pull Request Description

Our use relay on the behavior that completion and set-image block should happend during one run loop without add it to the end of main queue. This means we should use dispatch_main_async_safe to dispatch queue when current queue is main queue.

This behavior is changed during #2047. At that time I didn't realized that completion and set-image block should immediately call when current queue is main queue. So we then do code refactor about this logic.

dispatch_queue_t targetQueue = shouldUseGlobalQueue ? dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0) : dispatch_get_main_queue();

dispatch_async(targetQueue, ^{});

In fact, we should do like this:

if (shouldUseGlobalQueue) {
    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0), ^{
        [sself sd_setImage:targetImage imageData:targetData basedOnClassOrViaCustomSetImageBlock:setImageBlock];
        dispatch_main_async_safe(callCompletedBlockClojure);
    });
} else {
  // If current queue is already main queue, this will become all immediately block call instead of adding block to queue
    dispatch_main_async_safe(^{
        [sself sd_setImage:targetImage imageData:targetData basedOnClassOrViaCustomSetImageBlock:setImageBlock];
        dispatch_main_async_safe(callCompletedBlockClojure);
    });
}

This can work, but not looks well. I think we can create a more common usage macro, like this, to solve this kind of use case:

#define dispatch_queue_async_safe(queue, block)\
    if (strcmp(dispatch_queue_get_label(DISPATCH_CURRENT_QUEUE_LABEL), dispatch_queue_get_label(queue)) == 0) {\
        block();\
    } else {\
        dispatch_async(queue, block);\
    }

#define dispatch_main_async_safe(block) dispatch_queue_async_safe(dispatch_get_main_queue(), block)

Then we just need to change it to this:

dispatch_queue_async_safe(targetQueue, ^{});

I don't think this is a break API change. But if someone think this change should not happened on 4.2.2 patch release. I can change back to that first one in description to avoid modify any header file.

Any idea ?

@dreampiggy dreampiggy changed the title Fix that completion block and set image block are called asynchronously in FLAnimatedImageView Fix that completion block and set image block are called asynchronously for UIImageView+WebCache Nov 3, 2017
@dreampiggy dreampiggy changed the title Fix that completion block and set image block are called asynchronously for UIImageView+WebCache Fix that completion block and set image block are called asynchronously for UIView+WebCache Nov 3, 2017
…ly in UIView+WebCache. Add a more common macro to do current queue check
@dreampiggy dreampiggy force-pushed the fix_completion_block_memory_called_next_runloop branch from 66f51ed to 0eff98e Compare November 3, 2017 20:16
@dreampiggy dreampiggy added this to the 4.2.2 milestone Nov 3, 2017
@codecov-io
Copy link

Codecov Report

Merging #2093 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2093      +/-   ##
==========================================
- Coverage    78.3%   78.22%   -0.09%     
==========================================
  Files          35       35              
  Lines        3383     3384       +1     
==========================================
- Hits         2649     2647       -2     
- Misses        734      737       +3
Impacted Files Coverage Δ
SDWebImage/UIView+WebCache.m 59.37% <100%> (ø) ⬆️
SDWebImage/SDWebImageCompat.h 100% <100%> (ø) ⬆️
SDWebImage/SDWebImageWebPCoder.m 84.84% <0%> (-1.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2e5317...0eff98e. Read the comment docs.

@bpoplauschi
Copy link
Member

Looks good @dreampiggy. I will merge it.

@bpoplauschi bpoplauschi merged commit 821c2f3 into SDWebImage:master Nov 3, 2017
@bpoplauschi bpoplauschi deleted the fix_completion_block_memory_called_next_runloop branch November 3, 2017 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants