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

Feature - Appending Response Serializers #2862

Merged
merged 1 commit into from Jul 9, 2019

Conversation

@cnoon
Copy link
Member

commented Jul 8, 2019

Appending a response serializer after the request has been completed now resumes the request and processes the new response serializer as if it was added prior to it finishing.

Issue Link 馃敆

This PR solves an issue where it is currently possible to complete a request before appending a response serializer. There are two known cases where this behavior can manifest:

  1. Failure in URLRequestConvertible creation
  2. Cached URL response

Goals 鈿斤笍

Clients should be able to leverage cached URL responses as well as receive URLRequestConvertible errors directly whether the Request completes before the response serializer has been added or not. This is how Alamofire has behaved since the beginning.

Implementation Details 馃毀

When we added retry support for response serializers, we decided to not allow response serializers to be appended to a finished request due to complexity. We thought that it would be difficult to manage the request state and that you could end up with odd behaviors if a previous response serializer had already completed.

It turned out that that approach was a bit too heavy handed. Since Alamofire automatically starts creating the request when the request API is called, it's possible for the Request to be finished before a response serializer is added. Therefore, we need to alter the logic.

By allowing the Request to be resumed after it has finished when a new response serializer is added, we restore the previous behavior and also eliminate the race condition. The only downside of this approach is that the new response serializer could trigger a refresh and the previous response serializers will no longer be notified since they have been cleared. However, this is something that will rarely ever happen since 99.9% of clients will only ever use one response serializer. For those that do use more than one, they will almost certainly be adding them at the same time which will not hit this case.

The only way that you would hit a case where a previous response serializer would not be called in a retry scenario is if you would add an additional response serializer in a completion closure of a previous response serializer that triggered a retry. This is such a specific, advanced use case that I feel confident that the developer would understand exactly what they are trying to do.

Tests

I modified the tests to verify that appending response serializers inside and outside response serializers closures behaves as expected.

@cnoon cnoon added this to the 5.0.0-rc.1 milestone Jul 8, 2019

@cnoon cnoon requested a review from jshier Jul 8, 2019

@cnoon cnoon self-assigned this Jul 8, 2019

@jshier

jshier approved these changes Jul 9, 2019

Copy link
Contributor

left a comment

This looks good to me. We'll just have to be sure to call out this caveat in the documentation, as it will be difficult for users to detect what's causing two separate response serializers on the same request to return different responses.

@cnoon cnoon merged commit 0bab25f into master Jul 9, 2019

1 check failed

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

@cnoon cnoon deleted the feature/appending-response-serializer branch Jul 9, 2019

jshier added a commit that referenced this pull request Jul 10, 2019

jshier added a commit that referenced this pull request Jul 15, 2019

Update cURL support for async request creation. (#2863)
* Connect events to cURL description.

* Clean up and add more tests.

* Add DataEncoding and KeyEncoding. (#2858)

* Appending response serializer now resumes request if finished to handle races (#2862)

jshier added a commit that referenced this pull request Jul 19, 2019

jshier added a commit that referenced this pull request Jul 19, 2019

Update cURL support for async request creation. (#2863)
* Connect events to cURL description.

* Clean up and add more tests.

* Add DataEncoding and KeyEncoding. (#2858)

* Appending response serializer now resumes request if finished to handle races (#2862)

toomasr added a commit to toomasr/Alamofire that referenced this pull request Aug 19, 2019

toomasr added a commit to toomasr/Alamofire that referenced this pull request Aug 19, 2019

Update cURL support for async request creation. (Alamofire#2863)
* Connect events to cURL description.

* Clean up and add more tests.

* Add DataEncoding and KeyEncoding. (Alamofire#2858)

* Appending response serializer now resumes request if finished to handle races (Alamofire#2862)
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.