Improve interrupts (for discussion) #7023
Conversation
|
From #7022 This is a little of a) Remove the stream + send rst_stream - and hope connection is healthy? |
okhttp/src/jvmMain/kotlin/okhttp3/internal/http/CallServerInterceptor.kt
Show resolved
Hide resolved
| if (flushHeaders) { | ||
| writer.flush() | ||
| return stream | ||
| } catch (iioe: InterruptedIOException) { |
There was a problem hiding this comment.
@swankjesse Alternatives here
a) If interrupted - don't call flush, instead write a RST_STREAM, and throw an InterruptedIOException. Keep connection healthy but generally make sure the
b) Do this work via taskrunner so it can't be interrupted. Catch the interruption in say CountDownLatch in the caller thread, and then cancel the stream.
There was a problem hiding this comment.
I don’t like having to handle interrupts throughout the code. It seems... arbitrary?
And the testing strategy here also seems like it’s precisely targeting a specific rare situation... useful for regression testing but unsuited to directing us to put interrupt recovery in all the places we might need it.
We should figure out what our invariants are; perhaps this:
- If the user thread doing a Call.execute() is interrupted, that call is canceled and resources are not leaked
- If the dispatcher thread doing a Call.enqueue() is interrupted, that call is canceled and resources are not leaked
- If any OkHttp-internal thread is interrupted... what do we do? Perhaps we should enter a minimum-level-of-service mode: stop populating the cache; stop pooling connections; etc. ? Ie. assume that OkHttp is being closed?
Or we could even even just cancel every Call after an OkHttp-internal thread is interrupted. I think our behavior needs to be undefined there because we can’t do our job if our threads are taken away from us.
|
(I suspect our most interesting bug in this space is not degrading the cache when a Call thread is interrupted.) |
|
OkHttp don't have good guarantees for the stack of the Okio Source, Sink after an interrupts.
This is tough, that call might have written to the Http/2 connection. That's the dangerous bit. I think in most other cases the Interrupt is interrupting from a separate Buffer read, that can't break the Http/2 connection state.
I like this.
That's the problem. This is primarily for a client (ExoPlayer) designed specifically to use Interrupts as a signal, I think we should define it, so that anything not matching that definition is a bug. Or we state, interrupts are 100% not supported and we change the ExoPlayer adapter to use enqueue and a CountDown latch or similar. Are most other interrupts sensibly handled? Http/1.1 -> Interrupt kills the connection. |
|
Version 2 of that comment is a doc! |
Relates to square/okhttp#3146. This was from #71. There is a draft PR https://github.com/square/okhttp/pull/7185/files which documents OkHttp's ideal handling of cancellation including interrupts. But a few key points 1) This is a target state, and OkHttp does not currently handle interrupts correctly. In the past this has been identified, and the advice is to avoid interrupts on Http threads, see discussion on square/okhttp#1902. Also an attempt at a fix here square/okhttp#7023 which wasn't in a form to land. 2) Even with this fixed, it is likely to never be optimal, because of OkHttp sharing a socket connection for multiple inflight requests. From square/okhttp#7185 ``` Thread.interrupt() is Clumsy ---------------------------- `Thread.interrupt()` is Java's built-in mechanism to cancel an in-flight `Thread`, regardless of what work it's currently performing. We recommend against using `Thread.interrupt()` with OkHttp because it may disrupt shared resources including HTTP/2 connections and cache files. In particular, calling `Thread.interrupt()` may cause unrelated threads' call to fail with an `IOException`. ``` This PR leaves the Loader/DataSource thread parked on a countdown latch, while this may seem wasteful and an additional context switch. However in practice the response isn't returned until the Http2Connection and Http2Reader have a response from the server and these means effectively parking in a `wait()` statement here https://github.com/square/okhttp/blob/9e039e94123defbfd5f11dc64ae146c46b7230eb/okhttp/src/jvmMain/kotlin/okhttp3/internal/http2/Http2Stream.kt#L140 PiperOrigin-RevId: 446652468
Relates to square/okhttp#3146. This was from androidx/media#71. There is a draft PR https://github.com/square/okhttp/pull/7185/files which documents OkHttp's ideal handling of cancellation including interrupts. But a few key points 1) This is a target state, and OkHttp does not currently handle interrupts correctly. In the past this has been identified, and the advice is to avoid interrupts on Http threads, see discussion on square/okhttp#1902. Also an attempt at a fix here square/okhttp#7023 which wasn't in a form to land. 2) Even with this fixed, it is likely to never be optimal, because of OkHttp sharing a socket connection for multiple inflight requests. From square/okhttp#7185 ``` Thread.interrupt() is Clumsy ---------------------------- `Thread.interrupt()` is Java's built-in mechanism to cancel an in-flight `Thread`, regardless of what work it's currently performing. We recommend against using `Thread.interrupt()` with OkHttp because it may disrupt shared resources including HTTP/2 connections and cache files. In particular, calling `Thread.interrupt()` may cause unrelated threads' call to fail with an `IOException`. ``` This PR leaves the Loader/DataSource thread parked on a countdown latch, while this may seem wasteful and an additional context switch. However in practice the response isn't returned until the Http2Connection and Http2Reader have a response from the server and these means effectively parking in a `wait()` statement here https://github.com/square/okhttp/blob/9e039e94123defbfd5f11dc64ae146c46b7230eb/okhttp/src/jvmMain/kotlin/okhttp3/internal/http2/Http2Stream.kt#L140 PiperOrigin-RevId: 446652468
No description provided.