Skip to content

Commit 80928e7

Browse files
yschimkeicbaker
authored andcommitted
Workaround for OkHttp Interrupt issues.
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
1 parent 4b8ff72 commit 80928e7

File tree

2 files changed

+38
-1
lines changed

2 files changed

+38
-1
lines changed

Diff for: RELEASENOTES.md

+1
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@
8989

9090
* Data sources:
9191
* Rename `DummyDataSource` to `PlaceHolderDataSource`.
92+
* Workaround OkHttp interrupt handling.
9293
* Remove deprecated symbols:
9394
* Remove `Player.Listener.onTracksChanged`. Use
9495
`Player.Listener.onTracksInfoChanged` instead.

Diff for: libraries/datasource_okhttp/src/main/java/androidx/media3/datasource/okhttp/OkHttpDataSource.java

+37-1
Original file line numberDiff line numberDiff line change
@@ -32,19 +32,25 @@
3232
import androidx.media3.datasource.DataSourceException;
3333
import androidx.media3.datasource.DataSpec;
3434
import androidx.media3.datasource.HttpDataSource;
35+
import androidx.media3.datasource.HttpDataSource.HttpDataSourceException;
36+
import androidx.media3.datasource.HttpDataSource.InvalidContentTypeException;
37+
import androidx.media3.datasource.HttpDataSource.InvalidResponseCodeException;
3538
import androidx.media3.datasource.HttpUtil;
3639
import androidx.media3.datasource.TransferListener;
3740
import com.google.common.base.Predicate;
3841
import com.google.common.net.HttpHeaders;
42+
import com.google.common.util.concurrent.SettableFuture;
3943
import java.io.IOException;
4044
import java.io.InputStream;
4145
import java.io.InterruptedIOException;
4246
import java.util.Collections;
4347
import java.util.HashMap;
4448
import java.util.List;
4549
import java.util.Map;
50+
import java.util.concurrent.ExecutionException;
4651
import okhttp3.CacheControl;
4752
import okhttp3.Call;
53+
import okhttp3.Callback;
4854
import okhttp3.HttpUrl;
4955
import okhttp3.MediaType;
5056
import okhttp3.OkHttpClient;
@@ -299,8 +305,9 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
299305
Request request = makeRequest(dataSpec);
300306
Response response;
301307
ResponseBody responseBody;
308+
Call call = callFactory.newCall(request);
302309
try {
303-
this.response = callFactory.newCall(request).execute();
310+
this.response = executeCall(call);
304311
response = this.response;
305312
responseBody = Assertions.checkNotNull(response.body());
306313
responseByteStream = responseBody.byteStream();
@@ -448,6 +455,35 @@ private Request makeRequest(DataSpec dataSpec) throws HttpDataSourceException {
448455
return builder.build();
449456
}
450457

458+
/**
459+
* This method is an interrupt safe replacement of OkHttp Call.execute() which can get in bad
460+
* states if interrupted while writing to the shared connection socket.
461+
*/
462+
private Response executeCall(Call call) throws IOException {
463+
SettableFuture<Response> future = SettableFuture.create();
464+
call.enqueue(
465+
new Callback() {
466+
@Override
467+
public void onFailure(Call call, IOException e) {
468+
future.setException(e);
469+
}
470+
471+
@Override
472+
public void onResponse(Call call, Response response) {
473+
future.set(response);
474+
}
475+
});
476+
477+
try {
478+
return future.get();
479+
} catch (InterruptedException e) {
480+
call.cancel();
481+
throw new InterruptedIOException();
482+
} catch (ExecutionException ee) {
483+
throw new IOException(ee);
484+
}
485+
}
486+
451487
/**
452488
* Attempts to skip the specified number of bytes in full.
453489
*

0 commit comments

Comments
 (0)