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 - Retry Redesign #2704

Merged
merged 8 commits into from Feb 7, 2019

Conversation

@cnoon
Copy link
Member

cnoon commented Jan 31, 2019

This PR is a variant of #2693 combined with #2694 and refactored to accommodate all the feedback.

Goals ⚽️

The goals of this PR are to:

  • Combine the RetryPolicy and RequestRetrier protocols under a unified API
  • Refactor the ConnectionLostRetryPolicy to be a subclass of ExponentialBackoffRetryPolicy
  • Refactor the RequestRetrier protocol to support a Result type in the completion to allow retriers to modify the error on the Request

Implementation Details 🚧

PR #2693 introduces the RequestInterceptor protocol which is a combination of the RequestAdapter and RequestRetrier protocols. It also adds support to all the top-level APIs for adding RequestInterceptor types to each Request to override the RequestInterceptor potentially attached to the Session. This is a very powerful feature that makes it much easier to swap authenticate systems for different requests made on the same Session.

Throughout this PR, most of #2693 is still intact.

PR #2694 introduces the RetryPolicy protocol which is a synchronous variant of the RequestRetrier protocol. Retry policies were originally designed to support retrying requests along with exponential backoff for different types of errors and status code encountered on idempotent requests. While this was originally designed as a separate concept from authentication retry, the feedback in #2694 made it apparent that there was a great deal of overlap between retriers and retry policies and that we should attempt to combine them.

Multiple Adapters and Retriers

The hardest part about this change was to figure out how to combine retriers and retry policies behind a unified retry API that was async. Once I let go of the idea that retry policies needed to be synchronous, the design started to take shape. First off, we'd need to rely on a single RequestRetrier protocol that everything would conform to. That meant we'd need to support an array of retriers, instead of a single one. Additionally, if we're going to support multiple retriers, we should also support multiple adapters.

I also knew that we still wanted the idea of a retry policy. The concept is different than an authentication system, but both are built on the concept of retrying a request. What I came up with was a couple of things.

Interceptor

The new Interceptor class conforms to RequestInterceptor, and allows clients to construct a RequestInterceptor that consists of multiple adapters and retriers.

open class Interceptor {
    public let adapters: [RequestAdapter]
    public let retriers: [RequestRetrier]

    public init(adapters: [RequestAdapter] = [], retriers: [RequestRetrier] = []) {
        self.adapters = adapters
        self.retriers = retriers
    }
}

This simple type is actually super powerful. I then extended it to conform to RequestAdapter and RequestRetrier and use recursion to iterate through the collections of adapters and retriers while maintain async behavior.

extension Interceptor: RequestRetrier {
    open func should(_ session: Session, retry request: Request, with error: Error, completion: @escaping (_ result: Result<TimeInterval>) -> Void) {
        completion(.success(0.0))
        should(session, retry: request, with: error, using: retriers, completion: completion)
    }

    private func should(
        _ session: Session,
        retry request: Request,
        with error: Error,
        using retriers: [RequestRetrier],
        completion: @escaping (_ result: Result<TimeInterval>) -> Void)
    {
        var pendingRetriers = retriers

        guard !pendingRetriers.isEmpty else { completion(.failure(error)); return }

        let retrier = pendingRetriers.removeFirst()

        retrier.should(session, retry: request, with: error) { result in
            switch result {
            case .success:
                completion(result)
            case .failure(let error):
                self.should(session, retry: request, with: error, using: pendingRetriers, completion: completion)
            }
        }
    }
}

This encapsulates the complexity of iterating through all the adapters and retriers in the single class, rather than driving this logic into the internal Session logic.

Retry Policy

The other really big change in this PR is refactoring ExponentialBackoffRetryPolicy to RetryPolicy and making it conform to RequestRetrier. That in combination with the new Interceptor type makes it really easy to create a RequestInterceptor type that has multiple retry policies in addition to an authentication system. I also refactored the ConnectionLostRetryPolicy to be a subclass of RetryPolicy which removes all the duplicate logic.

The final change worth calling out is the addition of the default HTTP methods, status codes, and URL error codes as static properties on the RetryPolicy. This allowed me a single place to add all the necessary documentation to explain and link out all the additional information necessary.

Testing Details 🔍

Some of the tests from the other two PRs were purposefully deleted in this PR to gather feedback on the design. Once we're aligned, I'll add the tests back in and add any new tests that we need before we push this through.

@cnoon cnoon self-assigned this Jan 31, 2019

@cnoon cnoon requested a review from jshier Jan 31, 2019

@cnoon cnoon force-pushed the feature/retry-redesign branch from 19b220d to 88bcb2f Jan 31, 2019

@jshier
Copy link
Contributor

jshier left a comment

Looks great, just a few bits, and some thoughts about separate adapters and retriers.

Show resolved Hide resolved Source/RequestInterceptor.swift Outdated
Show resolved Hide resolved Source/RetryPolicy.swift
Show resolved Hide resolved Source/RetryPolicy.swift
Show resolved Hide resolved Source/RetryPolicy.swift Outdated
Show resolved Hide resolved Source/RetryPolicy.swift Outdated
Show resolved Hide resolved Source/RequestInterceptor.swift Outdated
Show resolved Hide resolved Source/RequestInterceptor.swift
Show resolved Hide resolved Source/RequestInterceptor.swift Outdated
Show resolved Hide resolved Source/Request.swift
Show resolved Hide resolved Source/Request.swift

@jshier jshier added this to Idea in Alamofire 5 via automation Jan 31, 2019

@jshier jshier added this to the 5.X milestone Jan 31, 2019

@cnoon cnoon force-pushed the feature/retry-redesign branch from 48981b3 to b9d0b5d Jan 31, 2019

@cnoon

This comment has been minimized.

Copy link
Member Author

cnoon commented Jan 31, 2019

Okay @jshier, we should be all good to go here to merge. I still have to write tests around retry policies and more tests are retry behavior, but the logic should be sound. If you want to release this in beta 2 while I'm gone, go for it. Otherwise we can release beta 2 with this after I get the tests finished once I get back.

@SlaunchaMan

This comment has been minimized.

Copy link
Contributor

SlaunchaMan commented Feb 4, 2019

I just ran into this today. In my use case, I’m doing a fairly standard OAuth-style login with a native login view controller and the SSO manager is a request adapter/retrier. When an error occurs, I would like to vend a login view controller to the caller, so that it can manage presenting it, but with Alamofire 4’s retrier logic there is no way to pass an error to the final response handler. This looks like it would solve my problem!

@cnoon

This comment has been minimized.

Copy link
Member Author

cnoon commented Feb 5, 2019

Exactly @SlaunchaMan! We had a very similar challenge as well that this will solve.

@cnoon

This comment has been minimized.

Copy link
Member Author

cnoon commented Feb 7, 2019

Okay @jshier, all the necessary tests have been added. This PR should be good to go.

@jshier

jshier approved these changes Feb 7, 2019

Copy link
Contributor

jshier left a comment

One minor nit about an associated value name, but looks great! 👍

Show resolved Hide resolved Source/AFError.swift Outdated

@cnoon cnoon force-pushed the feature/retry-redesign branch from 6009b17 to f1a67df Feb 7, 2019

@cnoon cnoon merged commit 1058f75 into master Feb 7, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

Alamofire 5 automation moved this from Idea to Done Feb 7, 2019

@cnoon cnoon deleted the feature/retry-redesign branch Feb 7, 2019

@cnoon cnoon modified the milestones: 5.X, 5.0.0-beta.2 Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment