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

Fix #2682: Cancel all requests on session invalidation or deinit. #2728

Merged
merged 9 commits into from Mar 7, 2019

Conversation

Projects
None yet
1 participant
@jshier
Copy link
Contributor

jshier commented Feb 25, 2019

Issue Link 馃敆

#2682

Goals 鈿斤笍

This PR ensures that all Requests finish with a specific error when the owning Session is deinitd or the underlying URLSession is invalidated.

Implementation Details 馃毀

Previously, after the Alamofire 5 refactor, Requests were no longer properly being finished, due to the separation of the URLSessionDelegate implementation from the Session itself, which meant the Session would be deinitd by the time the SessionStateProvider could act on the invalidation.

This PR adds an additional SessionStateProvider method, cancelRequestsForSessionInvalidation(with error: Error?) and a new AFError case, invalidatedSession(error: Error?) to properly support the new failure.

Testing Details 馃攳

A test was added for the invalidation after deinit case. I'm not sure the actual session invalidation case can be tested.

@jshier jshier requested a review from cnoon Feb 25, 2019

@jshier

This comment has been minimized.

Copy link
Contributor Author

jshier commented Feb 25, 2019

Two unanswered questions remain:

  1. Should we finish or cancel ongoing Requests? Since the Session is going away, there's no way to call back from the URLSession delegates to do anything, so whatever we do has to happen immediately. However, just finishing the Requests seems potentially problematic.
  2. Do we want additional tests for active vs. inactive Requests, many simultaneous Requests?
delegate: SessionDelegate,
rootQueue: DispatchQueue,
startRequestsImmediately: Bool = true,

This comment has been minimized.

@jshier

jshier Feb 25, 2019

Author Contributor

Moving this parameter to follow Swift guideline stating defaulted parameters should come after non-defaulted ones.

self.requests = requests
self.tasks = tasks
var requests: [Request] {
return Array(tasksToRequests.values)

This comment has been minimized.

@jshier

jshier Feb 25, 2019

Author Contributor

This may have a performance impact, so we could just return the values collection, since all we need to do is loop over the contents.

@jshier jshier referenced this pull request Mar 4, 2019

Closed

Alamofire 5.0.0 beta memory leaks on `validate()` #2734

1 of 1 task complete
@jshier

This comment has been minimized.

Copy link
Contributor Author

jshier commented Mar 6, 2019

@cnoon Thinking about this more, this seems like only a partial solution, as it only works for Requests mapped to *Tasks. It's also racy, given that the *Task isn't created until after the async URLRequest creation. I think a full solution will require us to keep track of all in-flight Requests, but before we do that we need to properly define the Request lifetime and what in-flight means. I'm thinking some sort of weak storage, but I'm not sure if that guarantees the lifetimes we'd want. Any ideas?

In the mean time I think I'm going to refactor this PR to separate the invalidation error from the deinit error, given they're really separate causes. I'll ping you again when that's done. If this brief version works out, we should merge it soon.

@jshier

This comment has been minimized.

Copy link
Contributor Author

jshier commented Mar 7, 2019

I've updated this PR to fully fix the validator leaks as well as separate the error case for deinitialization from invalidation.

This isn't a full solution but fixes the biggest issues.

@jshier jshier marked this pull request as ready for review Mar 7, 2019

@jshier jshier merged commit 9ab2a22 into master Mar 7, 2019

1 check failed

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

@jshier jshier deleted the bug/2682-complete-on-invalidation branch Mar 7, 2019

@jshier jshier added this to the 5.0.0-beta.3 milestone Mar 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.