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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix - Cancellation Loop #2778

Merged
merged 3 commits into from Apr 1, 2019

Conversation

Projects
None yet
2 participants
@cnoon
Copy link
Member

commented Mar 30, 2019

This PR fixes an issue in the response serialization retry logic where calling cancel and a few other APIs would result in the response serializers running again.

Goals ⚽️

Fix the issue where calling request.cancel() inside response serializer would result in the response serializer running again.

Implementation Details 🚧

I should have realized this when I was building it, but it came up when moving our networking library onto AF5 and running the test suite over it. When we finish running all the response serializers and determine that we do not need to retry the request, we then execute all the response serializer completions sequentially. However, we don't exactly know what are in those completions. Therefore, we have to assume the worst and that they call back into the request to possibly do things like request.cancel(). I actually had a few unit tests doing this and found some dragons.

Solution is to clear out all the response serializers and completions from the mutable state prior to actually executing the completions. This is how the logic should have been implemented in the beginning.

Testing Details 🔍

I added a test that fails without this change demonstrating the issue.

@cnoon cnoon self-assigned this Mar 30, 2019

@cnoon cnoon requested a review from jshier Mar 30, 2019

@cnoon cnoon added this to the 5.0.0-beta.5 milestone Mar 30, 2019

@cnoon
Copy link
Member Author

left a comment

Need to make one change yet.

Show resolved Hide resolved Source/Request.swift Outdated
@@ -1104,7 +1104,6 @@
developmentRegion = en;
hasScannedForEncodings = 0;
knownRegions = (
English,

This comment has been minimized.

Copy link
@cnoon

cnoon Mar 30, 2019

Author Member

This was an issue Xcode caught and wanted to fix so I made the change.

@jshier

jshier approved these changes Apr 1, 2019

Copy link
Contributor

left a comment

Makes sense, but I have some questions about a longer term fix and the testing delay.

Show resolved Hide resolved Tests/SessionTests.swift
Show resolved Hide resolved Tests/SessionTests.swift

@cnoon cnoon merged commit 4df5912 into master Apr 1, 2019

1 check failed

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

@cnoon cnoon deleted the bugfix/cancellation-loop branch Apr 1, 2019

cysp added a commit to ScentreGroup/Alamofire that referenced this pull request Apr 11, 2019

Fixed cancellation loop in response serializer completion (Alamofire#…
…2778)

* Fixed localization issue in project picked up by Xcode

* Fixed issue when cancelling request inside response serializer completion

* Pulled execution of response serializer completions outside of mutableState lock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.