Execute cancel block in the _URLRequestQueue

[sooth-sayer]

Fixes #10274

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @skatpgusskat and @javache to be potential reviewers.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[ide]

@javache this seems roughly right -- we want the cancellation block, or at least parts of it, to run on the URL request queue. I'm not sure this specific solution is the best way to go about things, though, since the cancellation block modified in this PR is sometimes wrapped in other cancellation blocks that we might want to run on the URL request queue too.

With this PR: other methods' cancellation blocks invoke _loadImageOrDataWithURLRequest's cancellation block, which dispatches to the URL request queue and runs more cancellation code.

What I'm thinking: other methods' (specifically public methods) cancellation blocks dispatch to the URL request queue and invoke _loadImageOrDataWithURLRequest's cancellation block, which doesn't do a dispatch (maybe it assets which queue it is running on). That way all cancellation code consistently runs on the URL request queue, and we're not nesting dispatch_async calls either.

[sooth-sayer]

@ide In some cases (https://github.com/facebook/react-native/blob/master/Libraries/Image/RCTImageLoader.m#L342) we must not call cancellation block returned by _loadImageOrDataWithURLRequest on the URL request queue. So, i think that _loadImageOrDataWithURLRequest is responsible for dispatching cancellation block to the appropriate queue.

[christopherdro]

Does this commit possible address the issue?
1a62b66

[gre]

@christopherdro maybe someone else can confirm this, but I've actually been able to still reliably trigger the error even on current master including the commit you mentioned. More precisely, after 1a62b66 I was still able to trigger the bug, but I noticed it was a bit harder to trigger. So my guess is this commit was incomplete on covering all breaking cases.

After applying this PR I can't anymore trigger the bug, so thanks to @sooth-sayer.

[javache]

Looks reasonable but could you structure it like this? Also, you'll need to use weakSelf / strongSelf to access _URLRequestQueue? Right now this will retain self.

return ^{
  typeof(self) strongSelf = weakSelf;
  if (strongSelf && !cancelled) {
    dispatch_async(strongSelf->_URLRequestQueue, ^{
      if (cancelLoad) {
        cancelLoad();
        cancelLoad = nil;
     }
  });
  OSAtomicOr32Barrier(1, &cancelled);
};

[javache]

Even better would be to put this dispatch inside the block that does the actual cancelling.

[facebook-github-bot]

@sooth-sayer updated the pull request - view changes

[sooth-sayer]

@javache Do you mean that we should put dispatch inside

if (cancelLoad) { 
  ...
} 

?

[javache]

I've got a fix up for this internally which includes your changes. I'll try land it later today, thanks!

[sooth-sayer]

@javache Ok, 👍

[avilyn]

@javache Are you able to estimate when a version will be released that includes this fix? Is that what you meant by "land it later today" above?

[ide]

@avilyn if it lands today or in the next few days it will go out with 0.37 (ETA 4 weeks) and we would probably cherry pick it back into 0.36 (ETA 2 weeks, currently in RC so you can use it now).

[avilyn]

@ide Perfect, thank you.

[javache]

Landed in 26be005 I'll ask for it to be picked 0.36 too.