Skip to content

Commit

Permalink
Workaround for OkHttp Interrupt issues.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
yschimke authored and icbaker committed May 5, 2022
1 parent 3701e80 commit 0f502b2
Showing 1 changed file with 37 additions and 1 deletion.
Expand Up @@ -29,21 +29,27 @@
import com.google.android.exoplayer2.upstream.DataSourceException;
import com.google.android.exoplayer2.upstream.DataSpec;
import com.google.android.exoplayer2.upstream.HttpDataSource;
import com.google.android.exoplayer2.upstream.HttpDataSource.HttpDataSourceException;
import com.google.android.exoplayer2.upstream.HttpDataSource.InvalidContentTypeException;
import com.google.android.exoplayer2.upstream.HttpDataSource.InvalidResponseCodeException;
import com.google.android.exoplayer2.upstream.HttpUtil;
import com.google.android.exoplayer2.upstream.TransferListener;
import com.google.android.exoplayer2.util.Assertions;
import com.google.android.exoplayer2.util.Util;
import com.google.common.base.Predicate;
import com.google.common.net.HttpHeaders;
import com.google.common.util.concurrent.SettableFuture;
import java.io.IOException;
import java.io.InputStream;
import java.io.InterruptedIOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.ExecutionException;
import okhttp3.CacheControl;
import okhttp3.Call;
import okhttp3.Callback;
import okhttp3.HttpUrl;
import okhttp3.MediaType;
import okhttp3.OkHttpClient;
Expand Down Expand Up @@ -281,8 +287,9 @@ public long open(DataSpec dataSpec) throws HttpDataSourceException {
Request request = makeRequest(dataSpec);
Response response;
ResponseBody responseBody;
Call call = callFactory.newCall(request);
try {
this.response = callFactory.newCall(request).execute();
this.response = executeCall(call);
response = this.response;
responseBody = Assertions.checkNotNull(response.body());
responseByteStream = responseBody.byteStream();
Expand Down Expand Up @@ -428,6 +435,35 @@ private Request makeRequest(DataSpec dataSpec) throws HttpDataSourceException {
return builder.build();
}

/**
* This method is an interrupt safe replacement of OkHttp Call.execute() which can get in bad
* states if interrupted while writing to the shared connection socket.
*/
private Response executeCall(Call call) throws IOException {
SettableFuture<Response> future = SettableFuture.create();
call.enqueue(
new Callback() {
@Override
public void onFailure(Call call, IOException e) {
future.setException(e);
}

@Override
public void onResponse(Call call, Response response) {
future.set(response);
}
});

try {
return future.get();
} catch (InterruptedException e) {
call.cancel();
throw new InterruptedIOException();
} catch (ExecutionException ee) {
throw new IOException(ee);
}
}

/**
* Attempts to skip the specified number of bytes in full.
*
Expand Down

0 comments on commit 0f502b2

Please sign in to comment.