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 - Response Serializer Retry Support #2716

Merged
merged 11 commits into from Mar 27, 2019

Conversation

Projects
None yet
2 participants
@cnoon
Copy link
Member

commented Feb 12, 2019

This PR demonstrates one potential way we can add retry support to response serializers. This approach is definitely a work in progress, so please keep that in mind. The test suite is passing and the approach is sound, it just requires us to make some decisions on how certain edge cases should behave.

Issue Link 馃敆

The design flaw has several forms, but generally results in users having to parse JSON payloads in Validation closures to get the errors extracted to all the request to be retried. Issue #2214 is one example of many use cases.

Please link other related issues if you know of them. I'll try to find some more later.

Goals 鈿斤笍

To add retry support to response serializers without changing the public APIs. Response serializers should be able to retry requests with a RequestRetrier when serialization errors occur.

Implementation Details 馃毀

The implementation of this isn't too bad amazingly.

Threading Model

The biggest change is around the threading model. Instead of tacking all the response serializers into an operation queue, we need to keep track of them as a collection of closures. I've created a simple responseSerializers array in the MutableState of a Request where we can append them. I also added append, next, and remove response serializer functions to safely access the response serializers in thread-safe ways.

I'm not at all tied to these APIs, just needed to put something together to demonstrate one way to do this.

Response Serializers

The response serializer closures are a bit different now than they were. First off, the closures are simply appended to the array of responseSerializers in the Request. They are no longer appended to the internalQueue. This allows us to iterate through them instead of resuming the queue.

The next change in them is that for both the DataRequest and DownloadRequest APIs where they take a responseSerializer, we call a retryRequest(:dueTo:completion:) API that will retry the Request if the RequestRetrier determines it should be retried. This is the exact same API that we use for retrying requests after validation.

If the retrier determines that the request should be retried, the request is restarted and all the response serializers stay in the array. If a response serializer completes successfully, its completion closure is called and it is removed from the array. This behavior presents some interesting questions in terms of multiple response serializers:

  1. What if the next response serializer throws an error that trips retry?
  • Does it make sense to retry after the first response serializer has succeeded and been removed?
  • Should all response serializers be executed until an error trips retry or they all succeed?
  1. If any response serializer trips retry, should they all be executed the next time around?
  • Currently, a successful response serializer is popped from the queue when it succeeds
  1. If you can retry from a response serializer, does it make sense to allow multiple response serializers?
  • Supporting multiple greatly overcomplicates things
  • It's not difficult to make a custom response serializer that makes multiple passes if necessary

IMO, it is really weird if a response serializer succeeds and is completed, then a second one fails, the request is retried, and the first response serializer is unaware that that happened. However, I do understand that that might make sense in some cases. Personally, I would implement that as a single response serializer that might need to make multiple passes on the payload. This would allow me to call a single completion with a custom payload type, but that's just what I would do. I don't completely understand all the use cases for people using multiple response serializers for a single call, nor do I think it wise to add a precondition to only allow a single response serializer per request.

If anyone has some really good examples of use cases where multiple response serializers make perfect sense, I'd love to hear them. The more info we have here the better the implementation we can land on for everyone.

Testing Details 馃攳

This PR is a work in progress, so I do not want to build out specific test cases until we've landed on the approach. Currently, the entire test suite is passing as is, so the functionality is sound. However, we do not currently have tests that exercise all the current edge cases.


Updates

I've now been able to finalize this PR and have some answers to the questions posed above that are worth documenting in the PR.

  1. What if the next response serializer throws an error that trips retry?

Completion closures are only called once all response serializers have completed. We never want to call the first response serializer's completion closure, then have the second response serializer trip a retry, and then the first response serializer is just out-of-luck.

  1. If any response serializer trips retry, should they all be executed the next time around?

Yes, they will all be executed again. The response serializer completion closures will only be executed once all response serializers have completed.

  1. If you can retry from a response serializer, does it make sense to allow multiple response serializers?

We are still going to allow it. It's difficult to foresee use cases where you would use more than one, but we've decided to continue to support it in AF5.

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

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

This looks pretty good. I've added some specific feedback. Functionally this is great, I just think some cleaner separation is in order.

@cnoon cnoon force-pushed the feature/response-serializer-retry-support branch from 2489892 to 521023f Mar 26, 2019

@cnoon cnoon changed the title [WIP] Feature - Response Serializer Retry Support Feature - Response Serializer Retry Support Mar 26, 2019

@cnoon
Copy link
Member Author

left a comment

Overall, I really like this approach and I think it is an awesome addition to Alamofire 5.

Show resolved Hide resolved Source/Request.swift
Show resolved Hide resolved Source/Request.swift
Show resolved Hide resolved Source/Request.swift Outdated
Show resolved Hide resolved Source/Request.swift
Show resolved Hide resolved Source/Request.swift
Show resolved Hide resolved Source/ResponseSerialization.swift Outdated
Show resolved Hide resolved Source/ResponseSerialization.swift Outdated
Show resolved Hide resolved Tests/SessionTests.swift

@cnoon cnoon added this to the 5.0.0-beta.4 milestone Mar 26, 2019

This was referenced Mar 26, 2019

@jshier
Copy link
Contributor

left a comment

Really looking great, just a few cleanup bits and questions.

Show resolved Hide resolved Source/Request.swift
Show resolved Hide resolved Source/Request.swift Outdated
Show resolved Hide resolved Source/Request.swift Outdated
Show resolved Hide resolved Source/ResponseSerialization.swift Outdated
Show resolved Hide resolved Source/Request.swift Outdated
Show resolved Hide resolved Source/ResponseSerialization.swift Outdated
Show resolved Hide resolved Source/ResponseSerialization.swift Outdated
Show resolved Hide resolved Source/ResponseSerialization.swift Outdated
Show resolved Hide resolved Source/Session.swift Outdated
Show resolved Hide resolved Tests/SessionTests.swift
@jshier

jshier approved these changes Mar 27, 2019

Copy link
Contributor

left a comment

Two small comments, but otherwise approved. This will be great!

Show resolved Hide resolved Source/Session.swift
Show resolved Hide resolved Source/Request.swift Outdated

@cnoon cnoon merged commit 4b2c33c into master Mar 27, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
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.