fix: correctly handle async cancellation of call context in OkHttp engine #1063
Conversation
| @@ -51,6 +51,7 @@ public class OkHttpEngine( | |||
| val engineResponse = mapOkHttpExceptions { engineCall.executeAsync() } | |||
|
|
|||
| callContext.job.invokeOnCompletion { | |||
| engineCall.cancel() | |||
There was a problem hiding this comment.
correctness: I'm not sure we want to trigger a cancellation on every invocation including successful ones. This may trigger other behaviors/paths in okhttp (including lifecycle methods on interceptors).
What we probably want is to know if there was a response body and if it was consumed or not. If not then cancel and close otherwise we don't need to do anything.
Alternatively we have the right hooks in place on HttpCall. We could add an internal open fun cleanup() { } that we override to invoke engineResponse.body.close() on the same thread that the execution is happening on.
There was a problem hiding this comment.
Yeah that makes sense. Let me try modifying HttpCall first and see how that works. Thanks!
There was a problem hiding this comment.
Discussed offline and decided to try more intelligent invokeOnCompletion logic instead.
| val response = engineResponse.toSdkResponse() | ||
| val requestTime = Instant.fromEpochMilliseconds(engineResponse.sentRequestAtMillis) | ||
| val responseTime = Instant.fromEpochMilliseconds(engineResponse.receivedResponseAtMillis) | ||
|
|
||
| return OkHttpCall(request, response, requestTime, responseTime, callContext, engineCall) | ||
| return OkHttpCall(request, response, requestTime, responseTime, callContext, engineCall).also { call -> | ||
| callContext.job.invokeOnCompletion { error -> |
There was a problem hiding this comment.
nit(naming): error -> cause to match documentation?
| return OkHttpCall(request, response, requestTime, responseTime, callContext, engineCall) | ||
| return OkHttpCall(request, response, requestTime, responseTime, callContext, engineCall).also { call -> | ||
| callContext.job.invokeOnCompletion { error -> | ||
| if (error != null) call.cancelInFlight() |
There was a problem hiding this comment.
correctness: you can receive a non-null CancellationException which is not meant to be treated as an error. do we still want to cancel the call in this case?
There was a problem hiding this comment.
Yes we definitely want to cancel when we get a CancellationException. We're bridging two contexts here so we want to ensure resources belonging to the engine are cleaned up when the call context is cancelled. I'll leave a comment to that effect for future readers.
| coroutineScope { | ||
| val parentScope = this | ||
|
|
||
| println("Invoking call on ctx $coroutineContext") |
Issue #
Resolves #1061
Description of changes
Update OkHttp engine call context cancellation to first cancel the call before attempting to close any response body.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.