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

Automatic Error Recovery #45

Merged
merged 12 commits into from
May 18, 2018
Merged

Automatic Error Recovery #45

merged 12 commits into from
May 18, 2018

Conversation

wmcginty
Copy link
Contributor

@wmcginty wmcginty commented May 17, 2018

This is another large feature that I want to get in before 2.0 pre-release if possible. Essentially, this does the following:

  • Adds a protocol Recoverable, which dictates when an arbitrary operation can be recovered from and re-attempted. Also adds an extension on Recoverable for easily preparing subsequent attempts.
  • Adds an optional property to the default BackendService (should it be a part of the BackendServiceProtocol?) for a RequestRecoveryStrategy. This object allows for certain errors to be handled automatically by returning a specific RecoveryDisposition of retry or fail.

For example: If the error returned is a 401 unauthorized, the recovery strategy can add the appropriate authorization header to the request, and return retry.

I can look into a subspec if we think that it belongs in one.

@wmcginty wmcginty changed the title Auatomic Error Recovery Automatic Error Recovery May 17, 2018
@codecov-io
Copy link

codecov-io commented May 17, 2018

Codecov Report

Merging #45 into release/2.0.0 will decrease coverage by 0.63%.
The diff coverage is 91.27%.

Impacted file tree graph

@@                Coverage Diff                @@
##           release/2.0.0      #45      +/-   ##
=================================================
- Coverage          96.91%   96.27%   -0.64%     
=================================================
  Files                 31       33       +2     
  Lines               1068     1208     +140     
=================================================
+ Hits                1035     1163     +128     
- Misses                33       45      +12
Impacted Files Coverage Δ
...es/Hyperspace/Service/Network/NetworkService.swift 100% <ø> (ø) ⬆️
Tests/Helper/Mocks/MockDecodableContainer.swift 100% <ø> (ø) ⬆️
Tests/Helper/Mocks/MockBackendService.swift 42.85% <0%> (-17.15%) ⬇️
...Hyperspace/Service/Recovery/RecoveryStrategy.swift 100% <100%> (ø)
...erspace/Service/Backend/BackendServiceHelper.swift 100% <100%> (ø) ⬆️
Sources/Hyperspace/Request/NetworkRequest.swift 100% <100%> (ø) ⬆️
Sources/Hyperspace/Request/AnyNetworkRequest.swift 100% <100%> (ø) ⬆️
Tests/NetworkServiceTests.swift 100% <100%> (ø) ⬆️
Tests/RecoverableTests.swift 94.87% <94.87%> (ø)
...es/Hyperspace/Service/Backend/BackendService.swift 96.87% <95.45%> (-3.13%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81fb628...593ac1a. Read the comment docs.

BackendServiceHelper.handleResponse(response, completion: completion)

case .failure(let error):
guard let recoveryStrategy = self?.recoveryStrategy else { return completion(.failure(error)) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the else on this guard, do we need to pipe it through BackendServerHelper so that the completion block gets run on the main thread?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. We very much do. Will tweak that now.

@wmcginty wmcginty merged commit 97e5f4b into release/2.0.0 May 18, 2018
@wmcginty wmcginty deleted the errorRecovery branch May 18, 2018 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants