- Notifications
You must be signed in to change notification settings - Fork9.2k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Canceling and Interrupting doc#7185
base:master
Are you sure you want to change the base?
Conversation
I’m not ready to merge this because this document describes behavior we haven’t yet implemented! @yschimke lets get consensus on what the behavior should be, implement it, and then merge a document describing it for our users? |
To implement this policy, we have code to limit the blast radius of untimely interruption, including | ||
checking the interrupt state before operating on shared resources. We have tests specifically | ||
confirming that precise interrupts don't permanently damage the response cache. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- Implement limiting blast radius on HTTP/2 connections + tests
- Implement limiting blast radius on disk cache + tests
- Don't permanently damage the response cache + tests
interrupted, we stop managing shared resources and fail all calls that follow. | ||
Once the connection pool thread is interrupted, OkHttp degrades the connection pool and cancels | ||
every call that attempts to use it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- Create a cache degraded state that throws
IOException
when accessed - Create a connection pool degraded state that throws
IOException
when accessed
**Precise interrupts** apply to a specific call thread. | ||
**Broad interrupts** apply to OkHttp-internal threads. This happens when a framework loops over | ||
threads it doesn't own and cancels them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
threads it doesn't own andcancels them. | |
threads it doesn't own andinterrupts them. |
**We discourage precise interrupts because they may have collateral damage.** Canceling a thread | ||
that happens to be writing to a shared HTTP/2 connection damages that connection for all calls | ||
sharing that connection. It may cause multiple unrelated calls that share that connection to fail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Based on this I'll reopen my existing PR. It implements this logic
// connection cannot be trusted anymore, partial writes are possible this.failConnection(iioe)
### Precise Interrupt Policy | ||
**We discourage precise interrupts because they may have collateral damage.** Canceling a thread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'll put up an ExoPlayer PR to use enqueue and CountdownLatch.
Thanks, I really like this. |
Relates tosquare/okhttp#3146. This was from#71.There is a draft PRhttps://github.com/square/okhttp/pull/7185/files which documents OkHttp's ideal handling of cancellation including interrupts.But a few key points1) 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 onsquare/okhttp#1902. Also an attempt at a fix heresquare/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.Fromsquare/okhttp#7185```Thread.interrupt() is Clumsy----------------------------`Thread.interrupt()` is Java's built-in mechanism to cancel an in-flight `Thread`, regardless ofwhat work it's currently performing.We recommend against using `Thread.interrupt()` with OkHttp because it may disrupt shared resourcesincluding HTTP/2 connections and cache files. In particular, calling `Thread.interrupt()` may causeunrelated 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 herehttps://github.com/square/okhttp/blob/9e039e94123defbfd5f11dc64ae146c46b7230eb/okhttp/src/jvmMain/kotlin/okhttp3/internal/http2/Http2Stream.kt#L140PiperOrigin-RevId: 446652468
Relates tosquare/okhttp#3146. This was fromandroidx/media#71.There is a draft PRhttps://github.com/square/okhttp/pull/7185/files which documents OkHttp's ideal handling of cancellation including interrupts.But a few key points1) 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 onsquare/okhttp#1902. Also an attempt at a fix heresquare/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.Fromsquare/okhttp#7185```Thread.interrupt() is Clumsy----------------------------`Thread.interrupt()` is Java's built-in mechanism to cancel an in-flight `Thread`, regardless ofwhat work it's currently performing.We recommend against using `Thread.interrupt()` with OkHttp because it may disrupt shared resourcesincluding HTTP/2 connections and cache files. In particular, calling `Thread.interrupt()` may causeunrelated 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 herehttps://github.com/square/okhttp/blob/9e039e94123defbfd5f11dc64ae146c46b7230eb/okhttp/src/jvmMain/kotlin/okhttp3/internal/http2/Http2Stream.kt#L140PiperOrigin-RevId: 446652468
okeegan commentedNov 25, 2024
@swankjesse@yschimke are you able to pick this up in the future? |
No description provided.