Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bugfix - Cancel Race Conditions #372

Merged
merged 2 commits into from
Aug 28, 2019
Merged

Conversation

cnoon
Copy link
Member

@cnoon cnoon commented Aug 28, 2019

This PR is a follow-up to #368 and #371 that is more comprehensive. After digging in further with @jshier, I was able to create a set of stress tests to test all the odd edge cases including:

  • Large image downloader queues
  • Resuming queued requests by manually calling .resume() on the request through the request receipt
  • Cancellation of queued requests using request receipts
  • Cancellation of queued requests by cancelling directly on Request using the RequestReceipt: receipt.request.cancel()

These tests were able to reproduce somewhat reliably race condition logic errors in the previous logic.

Goals ⚽

  1. Remove all logic where the ImageDownloader was relying on a task to be present when it can no longer be relied upon safely.
  2. Remove logic where the ImageDownloader would early out in completion if the response did not contain a request used to cache the image when successful.

Implementation Details 🚧

These changes are simply removing the reliance on anything with request creation being synchronous. The stress tests were able to tease out the troublesome spots.

Testing Details 🔍

The big changes here are the stress tests. I've placed them directly in the test target for now since there are only 4 tests which hit the service 10 times each. Therefore, it's lightweight enough to not have to create a separate target.

These tests will break roughly 1 out of 4 tries when executing without the changes in the ImageDownloader. However, with these changes, I have yet to see any tests fail on AF beta 6 or 7.

I'm not completely convinced that there isn't an additional bug in Alamofire related to cancellation as well. I've spent a lot of time trying to track it down, but have not yet been able to 100% pinpoint the Alamofire logic as a contributor to the race. I'll continue to investigate further over the next couple of days to see if I can reliably reproduce the issue.

@cnoon cnoon added this to the 4.0.0-beta.4 milestone Aug 28, 2019
@cnoon cnoon requested a review from jshier August 28, 2019 05:50
@cnoon cnoon self-assigned this Aug 28, 2019
@cnoon cnoon merged commit 24ab6b9 into master Aug 28, 2019
@cnoon cnoon deleted the bugfix/cancel-race-conditions branch August 28, 2019 06:05
@kinarobin
Copy link

@cnoon Could you have a look for AFNetworking #4407 ? This is same issue.
Thank you very much!

ryndinol pushed a commit to ryndinol/AlamofireImage that referenced this pull request Apr 26, 2021
…mofire#372)

* Updated Alamofire submodule and pod dependency to 5.0.0-beta.7

* Fixed several issues related to async task creation in ImageDownloader
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants