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.

Sign up for GitHub

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

Canceling and Interrupting doc #7185

Draft

swankjesse wants to merge 1 commit into master from jwilson.0326.canceling_calls

Member

swankjesse commented Mar 26, 2022

No description provided.


Canceling and Interrupting doc

70e4e98

Member Author

swankjesse commented Mar 26, 2022

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?

swankjesse commented

View reviewed changes

docs/features/canceling_calls.md

+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.

Member Author

swankjesse Mar 26, 2022

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

swankjesse commented

View reviewed changes

docs/features/canceling_calls.md

+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.

Member Author

swankjesse Mar 26, 2022

Create a cache degraded state that throws IOException when accessed
Create a connection pool degraded state that throws IOException when accessed

yschimke reviewed

View reviewed changes

docs/features/canceling_calls.md

+**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.

Collaborator

yschimke Mar 27, 2022

Suggested change

threads it doesn't own and cancels them.

threads it doesn't own and interrupts them.

yschimke reviewed

View reviewed changes

docs/features/canceling_calls.md

+**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.

Collaborator

yschimke Mar 27, 2022

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)

yschimke reviewed

View reviewed changes

docs/features/canceling_calls.md


		### Precise Interrupt Policy

		We discourage precise interrupts because they may have collateral damage. Canceling a thread

Collaborator

yschimke Mar 27, 2022

I'll put up an ExoPlayer PR to use enqueue and CountdownLatch.

Collaborator

yschimke commented Mar 27, 2022

Thanks, I really like this.

square deleted a comment from 1999chato

yschimke mentioned this pull request

Workaround OkHttp lack of clean interrupt support androidx/media#71

Closed

2 tasks

icbaker pushed a commit to androidx/media that referenced this pull request


Workaround for OkHttp Interrupt issues.

80928e7

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

icbaker pushed a commit to google/ExoPlayer that referenced this pull request


Workaround for OkHttp Interrupt issues.

0f502b2

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

daraqui approved these changes

View reviewed changes

yschimke mentioned this pull request

Should runInterruptible throw InterruptedIOException as CancellationException? Kotlin/kotlinx.coroutines#3551

Closed

This was referenced May 20, 2023

Http2 Connection becomes stalled after interrupt during call.execute #7016

Closed

Implement correct interrupt handling #7841

Open

Connect Timeout - Thread.Interrupt() doesn't cancel call.execute() when we're stuck at the connection state #6611

Closed

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