Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cancelAllRequests API #2890

Merged
merged 4 commits into from Aug 7, 2019

Conversation

@jshier
Copy link
Contributor

commented Aug 6, 2019

Issue Link 馃敆

While there's no specific request, this is a question that comes up every so often, so it makes sense to add an API for it.

Goals 鈿斤笍

This PR adds a cancelAllRequests method to Session, which cancels all currently in flight Requests. It's a more limited version of a similar API on URLSession, in that it does't block additional Requests from being made while cancellation is occurring.

Implementation Details 馃毀

This PR adds var activeRequests: Set<Request> to Session, and Requests are added when created and removed when they run cleanup() after running the response serializers.

This PR also adds a workaround for the apparent fact that URLSessionTasks which are never resumed will never return URLSessionTaskMetrics. Given this is an important event for Alamofire to see, we now always call resume() on tasks before calling cancel(). This appears to guarantee metrics will always be generated for the task. However, on the 2019 OSes, as of beta 5, this is not 100% reliable in Alamofire, resulting in about 1 failure per 25,000 cancellations, at least according to my tests. Previous OS versions are 100% reliable, so I've reported this as a bug (FB6789976).

Testing Details 馃攳

I've added tests for cancelling 100 auto resumed and non resumed Requests, as well as a retry scenario. Let me know if any other scenarios are needed.

@jshier jshier requested a review from cnoon Aug 6, 2019

@jshier jshier added this to the 5.0.0.rc.1 milestone Aug 6, 2019

@cnoon
cnoon approved these changes Aug 7, 2019
Copy link
Member

left a comment

Looks good to me with a couple questions back.

Source/Session.swift Outdated Show resolved Hide resolved
Source/Session.swift Outdated Show resolved Hide resolved
Source/Session.swift Show resolved Hide resolved
@jshier

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@cnoon This is ready for another look.

@jshier jshier merged commit 9482ebf into master Aug 7, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@jshier jshier deleted the experiment/lifetime-tracking branch Aug 7, 2019

toomasr added a commit to toomasr/Alamofire that referenced this pull request Aug 19, 2019
Add cancelAllRequests API (Alamofire#2890)
* Add bulk request cancellation API.

* Resume tasks before cancelling to ensure we get metrics event.

* Make cancellation reliable by resuming before cancel.

* Add queue parameter, remove unnecessary self.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.