search

Alamofire / AlamofireImage

AlamofireImage is an image component library for Alamofire
MIT License
3.99k stars 523 forks source link

Bugfix - Dequeuing Queued Requests #371

Closed cnoon closed 5 years ago

cnoon commented 5 years ago

This PR fixes a nasty bug in the ImageDownloader dequeuing logic that wasn't addressed properly when updating AFI to operate with async Request creation.

Goals :soccer:

  1. Fix issue where a dequeued request is dropped if the task creation on the request has not yet completed.

Implementation Details :construction:

This change is pretty simple overall. It removes the check on the task which is completely invalid with AF5 since task creation is now async. It instead manually creates the concept of state.canTransitionTo(.resumed) using an internal Set instead of calling the canTransitionTo API directly since it's internal to AF.

The big thing to consider here is whether to open up the canTransitionTo API and make it public. IMO I think this use case clearly demonstrates the need to do exactly that.

Testing Details :mag:

I have been trying unsuccessfully to recreate a test that can exhibit this behavior. One of our test apps can reliably reproduce this issue, and this change certain resolves it. However, due to the racing nature of this problem, it is extremely difficult to reproduce. I was able to reproduce the issue as well by hacking the task property on Request to start returning nil after a few calls which results in the completion closures not being called for the download.

I'm sure open to ideas on how to get a test to reliably take a long time to be created while making fast network requests to trigger dequeue. We could possibly encode a giant chunk of data, but everything is still racing which will just result in a flaky test I'm afraid.