Fixed ASBasicImageDownloader to handle multiple requests with the same…

[victormayorov]

… URL

[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 cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[appleguy]

Thanks! I have a finished-but-incompletely-tested version of this locally, but it's awesome to have the community step in and finish the job more quickly than I could.

[appleguy]

Looks good overall, but importantly:

• It appears to me that you're recreating the download task for each new client asking for the resource. This isn't necessary, right? In ensuring we only ever create one task, it is very important to consider thread safety, and locking will be required.
• My implementation uses a small custom object for containing the three callback items (queue, progress, completion) and then puts those in an array. I think this will be measurably more efficient than using dictionaries, although I will not block the review on this because it is a very important correctness fix.

If you're able to ensure that the task is only created once and is thread-safe (so that a caller definitely gets its callback even if the task is finishing right around the moment it's putting in the download request), I'll merge this diff right away and prepare a 1.2.1 release.

Thanks so much, @victormayorov!

[victormayorov]

@appleguy, thanks for your comments!

Regarding new tasks for each request - it was implemented in similar way before. Now it creates new task only if another one has already finished. See -[ASBasicImageDownloaderContext createSessionTaskIfNecessaryWithBlock:] for details. ASDN::MutexLocker l(_propertyLock) inside this method ensures that sessionTask is synchronized.

Thank you, I'll be waiting for fix of this bug in 1.2.1 release.

[appleguy]

Oh cool! My quick skim of the code (while in a meeting lol) missed that part and looked like it was unconditionally created. That sounds perfect, will merge tonight.

[appleguy]

Ok, I'm actually so happy with your attention to detail here that I am going to merge from a mobile device without even testing it -- LOL. Seriously, thank you @victormayorov! I'll test tonight and aim to get a release out this weekend once I can tell which of two or three other important diffs can land in time.

[appleguy]

@victormayorov Can you test this with the Kittens app in the simulator? I'm getting crashes and may need to revert it unless it can be shown not to be caused by this diff.

Thread 1Queue : com.apple.main-thread (serial)
#0 0x000000010328900b in objc_msgSend ()
#1 0x00000001039eb716 in CGImageRetain ()
#2 0x00000001040c601e in -UIImage initWithCGImage:scale:orientation:
#3 0x00000001040c5dfd in +UIImage imageWithCGImage:
#4 0x0000000102a4de2e in 49-[ASNetworkImageNode _lazilyLoadImageIfNecessary]_block_invoke52 at /Users/scottg/code/AsyncDisplayKit/AsyncDisplayKit/ASNetworkImageNode.mm:214
#5 0x0000000102a4d3ce in __51-[ASNetworkImageNode _downloadImageWithCompletion:]_block_invoke at /Users/scottg/code/AsyncDisplayKit/AsyncDisplayKit/ASNetworkImageNode.mm:177
#6 0x00000001029de108 in __57-[ASBasicImageDownloaderContext completeWithImage:error:]_block_invoke at /Users/scottg/code/AsyncDisplayKit/AsyncDisplayKit/Details/ASBasicImageDownloader.mm:127
#7 0x0000000105490186 in _dispatch_call_block_and_release ()
#8 0x00000001054af614 in _dispatch_client_callout ()
#9 0x0000000105497a1c in _dispatch_main_queue_callback_4CF ()
#10 0x00000001035411f9 in __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE ()
#11 0x0000000103502dcb in CFRunLoopRun ()
#12 0x0000000103502366 in CFRunLoopRunSpecific ()
#13 0x0000000107857a3e in GSEventRunModal ()
#14 0x000000010408c900 in UIApplicationMain ()
#15 0x00000001029dcf1f in main at /Users/scottg/code/AsyncDisplayKit/examples/Kittens/Sample/main.m:18
#16 0x00000001054e3145 in start ()
#17 0x00000001054e3145 in start ()
Enqueued from NSOperationQueue 0x7fdcea567d60 :: NSOperation 0x7fdcec893590 (QOS: UTILITY) (Thread 13)Queue : NSOperationQueue 0x7fdcea567d60 :: NSOperation 0x7fdcec893590 (QOS: UTILITY) (serial)
#0 0x000000010549343f in _dispatch_barrier_async_f ()
#1 0x00000001029dde84 in -[ASBasicImageDownloaderContext completeWithImage:error:] at /Users/scottg/code/AsyncDisplayKit/AsyncDisplayKit/Details/ASBasicImageDownloader.mm:126
#2 0x00000001029dfd1d in -[ASBasicImageDownloader URLSession:downloadTask:didFinishDownloadingToURL:] at /Users/scottg/code/AsyncDisplayKit/AsyncDisplayKit/Details/ASBasicImageDownloader.mm:294
#3 0x0000000107e701a0 in __82-[NSURLSession delegate_downloadTask:didFinishDownloadingToURL:completionHandler:]_block_invoke ()
#4 0x0000000102eab57f in __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK ()
#5 0x0000000102dec0b2 in -NSBlockOperation main
#6 0x0000000102dce774 in -__NSOperationInternal _start:
#7 0x0000000102dce383 in __NSOQSchedule_f ()
#8 0x00000001054af614 in _dispatch_client_callout ()
#9 0x00000001054966a7 in _dispatch_queue_drain ()
#10 0x0000000105495cc0 in _dispatch_queue_invoke ()
#11 0x00000001054993b9 in _dispatch_root_queue_drain ()
#12 0x000000010549ab17 in _dispatch_worker_thread3 ()
#13 0x000000010581c637 in _pthread_wqthread ()
#14 0x000000010581a40d in start_wqthread ()

[victormayorov]

Hi, @appleguy ! Thank you, I found the reason. I was really surprised, but blocks doesn't capture CGImageRef, thus we've got pointer to deallocated variable. I fixed it and created new pull request #478

Thank you!

[markmark1]

Wow cool can't wait to try this release

On Thu, Jun 11, 2015 at 12:55 AM, victormayorov notifications@github.com
wrote:

Hi, @appleguy ! Thank you, I found the reason. I was really surprised, but blocks doesn't capture CGImageRef, thus we've got pointer to deallocated variable. I fixed it and created new pull request #478

Thank you!

Reply to this email directly or view it on GitHub:
#476 (comment)