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

Fix - SessionManager Retry Delay #1864

Merged
merged 1 commit into from Jan 14, 2017

Conversation

Projects
None yet
2 participants
@cnoon
Member

cnoon commented Dec 19, 2016

This PR addresses the issue pointed out in #1853 where the SessionManager internal retry logic for AdaptError instances was not respecting the time delay. This small change now respects the delay returned by the retrier.

@jshier

jshier approved these changes Dec 19, 2016

Functionally looks good, though I have a question.

@@ -874,12 +874,16 @@ open class SessionManager {
return
}
let retrySucceeded = strongSelf.retry(request)
DispatchQueue.utility.after(timeDelay) {

This comment has been minimized.

@jshier

jshier Dec 19, 2016

Contributor

I'm unclear as to whether we should be executing this code on a separate queue or the same queue that originally executed the request. Wouldn't this logic create a new queue for every retry attempt?

Perhaps at some point one of us should create a runtime diagram that outlines the various queues used...

@jshier

jshier Dec 19, 2016

Contributor

I'm unclear as to whether we should be executing this code on a separate queue or the same queue that originally executed the request. Wouldn't this logic create a new queue for every retry attempt?

Perhaps at some point one of us should create a runtime diagram that outlines the various queues used...

This comment has been minimized.

@cnoon

cnoon Dec 20, 2016

Member

Great question. All this logic does is dispatch after a given interval onto the utility queue. We're not creating a new queue. The utility queue is concurrent and is merely a thread pool that will execute once it has resources. Generally this will happen immediately, unless you have multiple higher priority operations running.

None of this retry logic executes on the calling thread. If you create a request that fails to be adapted (for the sake of argument let's say your OAuth token is expired), then the retry logic hops off the calling thread in the allowRetrier(_:toRetryRequest:withError:) API. Then all the retry logic is executed on the utility queue. Once the completion is called (we don't know what queue we'll be called on), then we dispatch to utility once more to allow the delay to pass before retrying. The retry logic simply re-adapts the request and hands the task over to the session to execute. The new task will then be executed on the URLSession's internal queue and all callbacks will again execute on the SessionDelegate queue.

I think putting together a queue diagram is a great idea and is something we should add to the Trello backlog.

@cnoon

cnoon Dec 20, 2016

Member

Great question. All this logic does is dispatch after a given interval onto the utility queue. We're not creating a new queue. The utility queue is concurrent and is merely a thread pool that will execute once it has resources. Generally this will happen immediately, unless you have multiple higher priority operations running.

None of this retry logic executes on the calling thread. If you create a request that fails to be adapted (for the sake of argument let's say your OAuth token is expired), then the retry logic hops off the calling thread in the allowRetrier(_:toRetryRequest:withError:) API. Then all the retry logic is executed on the utility queue. Once the completion is called (we don't know what queue we'll be called on), then we dispatch to utility once more to allow the delay to pass before retrying. The retry logic simply re-adapts the request and hands the task over to the session to execute. The new task will then be executed on the URLSession's internal queue and all callbacks will again execute on the SessionDelegate queue.

I think putting together a queue diagram is a great idea and is something we should add to the Trello backlog.

@@ -471,10 +471,10 @@ extension SessionDelegate: URLSessionTaskDelegate {
/// If an error occurred and the retrier is set, asynchronously ask the retrier if the request
/// should be retried. Otherwise, complete the task by notifying the task delegate.
if let retrier = retrier, let error = error {
retrier.should(sessionManager, retry: request, with: error) { [weak self] shouldRetry, delay in
retrier.should(sessionManager, retry: request, with: error) { [weak self] shouldRetry, timeDelay in

This comment has been minimized.

@jshier

jshier Dec 19, 2016

Contributor

This rename doesn't seem necessary, as the previous name wasn't ambiguous (aren't all delays in time?). But it doesn't necessarily need to be changed.

@jshier

jshier Dec 19, 2016

Contributor

This rename doesn't seem necessary, as the previous name wasn't ambiguous (aren't all delays in time?). But it doesn't necessarily need to be changed.

This comment has been minimized.

@cnoon

cnoon Dec 20, 2016

Member

The only reason I changed it was to match the "documented" name of the closure parameter. I also thought it would be good to match the same name in the SessionManager.

@cnoon

cnoon Dec 20, 2016

Member

The only reason I changed it was to match the "documented" name of the closure parameter. I also thought it would be good to match the same name in the SessionManager.

@jshier

This comment has been minimized.

Show comment
Hide comment
@jshier

jshier Dec 21, 2016

Contributor

👍

Contributor

jshier commented Dec 21, 2016

👍

@cnoon cnoon merged commit 25d8fdd into master Jan 14, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@cnoon cnoon deleted the fix/issue-1853-session-manager-retry-delay branch Jan 14, 2017

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