BottleRocketStudios / iOS-Hyperspace

An extremely lightweight wrapper around URLSession to make working with APIs a breeze.
Apache License 2.0
47 stars 17 forks source link

Max Recovery Attempts #136

Closed br-tyler-milner closed 3 years ago

br-tyler-milner commented 3 years ago

Should the default behavior be "no maximum" if a request's maxRecoveryAttempts isn't set? This seems to hammer the API if for whatever reason an endpoint isn't working correctly. In our app, we were seeing nonstop 401's and I'm honestly surprised we didn't kill the server. Maybe we should make automatic request recovery strictly opt-in? Or maybe just retry once as the default?

earlgaspard commented 3 years ago

Having retry once as the default would be an easier fix for the "no maximum" currently being set. A strictly opt-in solution seems like it would be more involved and could still be introduced later.

wmcginty commented 3 years ago

This is a good callout - we got bitten by this a while back, but because of a re-write being left on. Definitely agree adjusting Requests conformance would be a good starting point:

    /// The maximum number of attempts that this operation should make before completely aborting. This value is nil when there is no maximum.
    public var maxRecoveryAttempts: UInt?

    /// The number of recovery attempts that this operation has made
    public var recoveryAttemptCount: UInt = 1
earlgaspard commented 3 years ago

Should this be the fix:

/// The maximum number of attempts that this operation should make before completely aborting. A nil value means there is no maximum.
public var maxRecoveryAttempts: UInt? = 1

/// The number of recovery attempts that this operation has made.
public var recoveryAttemptCount: UInt = 0
earlgaspard commented 3 years ago

Or

/// The maximum number of attempts that this operation should make before completely aborting. A nil value means there is no maximum.
public var maxRecoveryAttempts: UInt? = RequestDefaults.defaultMaxRecoveryAttempts
br-tyler-milner commented 3 years ago

I like the option involving making it something we can configure on RequestDefaults (with a default value of "retry once").

earlgaspard commented 3 years ago

Resolved with PR 137