Fix combining auth and retry #1414
Conversation
JamesNK
force-pushed
the
jamesnk/retry-auth
branch
from
91ac83e to
2bd39b2
Compare
| } | ||
|
|
||
| if (CommitedCallTask.IsCompletedSuccessfully() && CommitedCallTask.Result == call) | ||
| finally |
There was a problem hiding this comment.
I noticed we don't do any except handling on the top-level of the StartCall here. Is it worthwhile to do here or is the exception handling at the call sites sufficient?
There was a problem hiding this comment.
All logic that can throw should be inside a try/catch.
However, a global catch is a good idea for safety. Also, throwing on CTS cancellation and reusing HandleUnexpectedError simplifies the code.
Updated.
| // There is no response task if there was a preemptive cancel. | ||
| CompatibilityHelpers.Assert(call.CancellationToken.IsCancellationRequested, "Request should have been made if call is not preemptively cancelled."); | ||
| call.CancellationToken.ThrowIfCancellationRequested(); | ||
| httpResponse = await call.HttpResponseTask.ConfigureAwait(false); |
There was a problem hiding this comment.
| httpResponse = await call.HttpResponseTask.ConfigureAwait(false); | |
| httpResponse = await call!.HttpResponseTask.ConfigureAwait(false); |
call is guaranteed to be set here, correct? Annotation makes it a little easier to read since it's nullable.
There was a problem hiding this comment.
Yes, a call is always set at this point.
I think an annotation makes it less clear what is going on. Since this compiles, code analysis guarantes that it's not null here. An annotation would make me wonder why this could be null.
2 participants
Fixes #1406
Adds a
TaskCompletionSource<HttpResponseMessage>so that the task to get the response is never null.Best reviewed with hide whitespace enabled.