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

Feature - Terminal Response Serializers #2810

Merged
merged 1 commit into from Apr 23, 2019

Conversation

Projects
None yet
2 participants
@cnoon
Copy link
Member

commented Apr 17, 2019

This PR fixes an issue that should be an extreme edge case of misuse where developers are adding a response serializer to an already completed request. Previously, the completion would never be called. It's now called with a specific response serialization error.

Goals ⚽️

Any response serializer added to a completed request should still have the completion called with a failure.

Implementation Details 🚧

Thankfully, the implementation here is pretty straightforward. We are now tracking a .finished state on Request. When a response serializer is appended to a Request, we now check if the Request is .finished. If it is, we update the error on the Request, and kick off another round of response serialization processing. This will clear out the response serializer and the process can continue as many times as the client desires.

Overall, no one should ever hit this use case. We only found this due to a test creating a request, cancelling it, resuming it, then adding a response serializer to it.

let request = session.request(urlRequest)
    .cancel()
    .resume()
    .response { resp in
        response = resp
        expectation.fulfill()
    }

This is not a case that we intend to support because it is explicitly misusing the public API. However, we wanted to ensure that the response serializer completion was still called with a clear error that it was added after the Request has already completed.

Previous Behavior and Retry Logic

This use case is supported in AF 4.x with the way the internal Request queue handles response serializer additions. In AF5, we've rebuilt the threading system, and also the response serialization system to support retry as well. We cannot support both retry from response serializers as well as executing addition response serializers after the request completes due to really weird edge cases that would crop up with retry logic.

For example, let's say I am using retry, and append two response serializers correctly. I execute the request, it goes through a retry, and the two completions are called. Then a user adds a 3rd response serializer to the Request. If we just executed it like normal, it "could" trigger a retry, but how would we do that? We've already called the completions on the first two response serializers. It wouldn't make sense to execute them again. It also wouldn't make sense to retry a Request based on the third response serializer's retry result without the other two response serializers knowing about it.

Because of these weird conditions, we've decided to have this simply result in an error. There are no conditions where you should be resuming your requests before adding response serializers through chaining when startRequestsImmediately is false. When it's true, You need to add your response serializers immediately after to ensure they are picked up before a cached response is pulled from the URLCache.

Testing Details 🔍

Added a failing test that demonstrated the issue and the fix is now passing.

@cnoon cnoon added this to the 5.0.0-beta.6 milestone Apr 17, 2019

@cnoon cnoon requested a review from jshier Apr 17, 2019

@cnoon cnoon self-assigned this Apr 17, 2019

@cnoon cnoon force-pushed the feature/terminal-response-serializer branch from 6381468 to 7402b8b Apr 23, 2019

Show resolved Hide resolved Source/AFError.swift
public enum State {
case initialized, resumed, suspended, cancelled
case initialized, resumed, suspended, cancelled, finished

This comment has been minimized.

Copy link
@cnoon

cnoon Apr 23, 2019

Author Member

I figured .finished was more correct than .completed based on the different naming conventions we've come up with in other places with Request and Task.

@jshier
Copy link
Contributor

left a comment

I think some more explicit checking around .cancelled is necessary as we discussed. We should have some additional tests in that case as well. Should probably also double check whether there are any other tests that examine the final state and ensure they're all updated with the newest expectations.

Show resolved Hide resolved Source/AFError.swift Outdated
Show resolved Hide resolved Source/AFError.swift
case (.resumed, .cancelled), (.suspended, .cancelled),
(.resumed, .suspended), (.suspended, .resumed): return true
case (.suspended, .suspended), (.resumed, .resumed): return false
case (.initialized, _):

This comment has been minimized.

Copy link
@jshier

jshier Apr 23, 2019

Contributor

I think the state machine need to be updated to ensure (or make explicit) we can only transition to finished from non-cancelled states.

This comment has been minimized.

Copy link
@cnoon

cnoon Apr 23, 2019

Author Member

I agree. I actually put that code in and it's covered by this case already:

case (_, .initialized), (.cancelled, _):
    return false

This comment has been minimized.

Copy link
@cnoon

cnoon Apr 23, 2019

Author Member

To make it more clear, I updated that case to this:

case (_, .initialized), (.cancelled, _), (.finished, _):
    return false
Show resolved Hide resolved Source/Request.swift
Show resolved Hide resolved Source/Request.swift Outdated

@cnoon cnoon force-pushed the feature/terminal-response-serializer branch from 7402b8b to 85a2120 Apr 23, 2019

@cnoon cnoon force-pushed the feature/terminal-response-serializer branch from 85a2120 to 656f331 Apr 23, 2019

@jshier

jshier approved these changes Apr 23, 2019

Copy link
Contributor

left a comment

👍 Looks good!

@jshier jshier merged commit 2154c66 into master Apr 23, 2019

1 check failed

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

@jshier jshier deleted the feature/terminal-response-serializer branch Apr 23, 2019

cysp added a commit to ScentreGroup/Alamofire that referenced this pull request May 2, 2019

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.