Workaround Photos.framework deadlock by locking

[Adlai-Holler]

Ugh, it kills me to have to do this but the framework will deadlock pretty quick otherwise once you start hammering it.

[Adlai-Holler]

Note: After more testing this isn't enough. Since the autorelease pool containing the PHFetchResult isn't getting drained inside the lock, it'll still eventually deadlock. Working on it…

[appleguy]

No worries, Apple frameworks are not always thread safe. I have some channels to request improvements of this nature, so if you collect enough data to file a Radar on it, I can reference that (or file one for you if you post it here).

[Adlai-Holler]

@appleguy Thanks man. I filed Radar #22984886 about the deadlocking.

I've modified the PR so that:

• Autorelease pool gets drained while holding fetch request lock, because -[PHFetchResult dealloc] contributes to the deadlocking
• Max 1 pending Photos.framework image request per node
• Image request operation doesn't keep node alive (oops)
• Image request operation gets cancelled in clearContents
• Max 10 concurrent image requests from ASDK
  • EDIT: High quality image requests, that is. Since fast format requests must be asynchronous, there may be more, but those are far less taxing so it doesn't present an issue in my testing.

This helped my production app handle ridiculous photos framework requirements pretty dern well.

[appleguy]

@Adlai-Holler really, really fantastic. I am surfacing your radar to get some attention on it. Who knows if it will be an iOS 9.x or 10.0, but eventually we should be able to install an OS check to enable more concurrency here. This should still be pretty darn fast compared to most people's implementation.

Will review & merge soon, probably later tonight. Are you fully integrated with -clearFetchedData, in addition to -clearContents?

[Adlai-Holler]

@appleguy You got it! I added the option for ASMultiplexDataSources to provide PHAssets if they're immediately available. This would be useful to me, because my application maintains an NSCache of existing assets. I also considered simply having a static NSCache but then we'd have to evict when photos library changes etc, which isn't our business.

In terms of -clearFetchedData, I wasn't sure. My rationale was, don't cancel the image request since we don't currently cancel the image download, but now that I type that out it's pretty weak sauce. Should I go ahead and cancel the image request there? Should I also cancel the image download as well?

Thanks so much for being responsive, I appreciate it a lot! ⚡

[appleguy]

@Adlai-Holler - good find, it feels like a bug to me (given that the network range is relatively new to OSS) that the image request would not be cancelled in -clearFetchedData. That's definitely one of the points of the method. Of course, we should probably be canceling the request in -clearContents as well, which theoretically should be called before -clearFetchedData in most cases anyway, but when content enters the network range and then scrolls out by a reversal of direction it's still appropriate to cancel it. Same for PH assets.

If you want to make that tweak I'm pretty sure this will be good to land when I have a chance to review.

[appleguy]

Should this really be conditional inside _shouldClearFetchedImageData? Given that it hasn't downloaded yet...a cancellation is a little different than clearing the fetched data, I'd say.