From f2276f613d8fd7dccf7a7f4428d679ae8e031e7e Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 23 Apr 2024 07:18:55 -0700 Subject: [PATCH] Fix SocketTimeoutException in `CronetDataSource`. In some versions of Cronet, the request provided by the callback is not always the same. This leads to callback not completing and request timing out. The fix changes from having one `UrlRequestCallback` per `CronetDataSource` to one `UrlRequestCallback` per `UrlRequest`. Everytime a current request is canceled, the current callback is closed. PiperOrigin-RevId: 627379153 --- RELEASENOTES.md | 4 + .../datasource/cronet/CronetDataSource.java | 92 +++++++++++++------ .../cronet/CronetDataSourceTest.java | 55 ++++++++--- 3 files changed, 108 insertions(+), 43 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 3ff8429290..38bcc31826 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -46,6 +46,10 @@ * Downloads: * OkHttp Extension: * Cronet Extension: + * Fix `SocketTimeoutException` in `CronetDataSource`. In some versions of + Cronet, the request provided by the callback is not always the same. + This leads to callback not completing and request timing out + (https://issuetracker.google.com/328442628). * RTMP Extension: * HLS Extension: * Fix bug where pending EMSG samples waiting for a discontinuity were diff --git a/libraries/datasource_cronet/src/main/java/androidx/media3/datasource/cronet/CronetDataSource.java b/libraries/datasource_cronet/src/main/java/androidx/media3/datasource/cronet/CronetDataSource.java index 300fc2e2ba..adb28409c6 100644 --- a/libraries/datasource_cronet/src/main/java/androidx/media3/datasource/cronet/CronetDataSource.java +++ b/libraries/datasource_cronet/src/main/java/androidx/media3/datasource/cronet/CronetDataSource.java @@ -22,6 +22,7 @@ import static org.chromium.net.UrlRequest.Builder.REQUEST_PRIORITY_MEDIUM; import android.net.Uri; import android.text.TextUtils; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import androidx.media3.common.C; import androidx.media3.common.MediaLibraryInfo; import androidx.media3.common.PlaybackException; @@ -55,6 +56,9 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.concurrent.Executor; +import org.checkerframework.checker.nullness.qual.EnsuresNonNull; +import org.checkerframework.checker.nullness.qual.RequiresNonNull; +import org.checkerframework.dataflow.qual.SideEffectFree; import org.chromium.net.CronetEngine; import org.chromium.net.CronetException; import org.chromium.net.NetworkException; @@ -441,8 +445,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { // The size of read buffer passed to cronet UrlRequest.read(). private static final int DEFAULT_READ_BUFFER_SIZE_BYTES = 32 * 1024; - /* package */ final UrlRequest.Callback urlRequestCallback; - private final CronetEngine cronetEngine; private final Executor executor; private final int requestPriority; @@ -467,6 +469,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { // Written from the calling thread only. currentUrlRequest.start() calls ensure writes are visible // to reads made by the Cronet thread. @Nullable private UrlRequest currentUrlRequest; + @VisibleForTesting @Nullable /* package */ UrlRequestCallback currentUrlRequestCallback; @Nullable private DataSpec currentDataSpec; // Reference written and read by calling thread only. Passed to Cronet thread as a local variable. @@ -510,7 +513,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { this.keepPostFor302Redirects = keepPostFor302Redirects; clock = Clock.DEFAULT; this.readBufferSize = readBufferSize; - urlRequestCallback = new UrlRequestCallback(); requestProperties = new RequestProperties(); operation = new ConditionVariable(); } @@ -576,8 +578,8 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { currentDataSpec = dataSpec; UrlRequest urlRequest; try { - urlRequest = buildRequestBuilder(dataSpec).build(); - currentUrlRequest = urlRequest; + createCurrentUrlRequestAndCallback(dataSpec); + urlRequest = currentUrlRequest; } catch (IOException e) { if (e instanceof HttpDataSourceException) { throw (HttpDataSourceException) e; @@ -817,10 +819,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { @UnstableApi @Override public synchronized void close() { - if (currentUrlRequest != null) { - currentUrlRequest.cancel(); - currentUrlRequest = null; - } + closeCurrentUrlRequestAndCallback(); if (readBuffer != null) { readBuffer.limit(0); } @@ -834,6 +833,18 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { } } + private void closeCurrentUrlRequestAndCallback() { + if (currentUrlRequest != null) { + currentUrlRequest.cancel(); + currentUrlRequest = null; + } + + if (currentUrlRequestCallback != null) { + currentUrlRequestCallback.close(); + currentUrlRequestCallback = null; + } + } + /** Returns current {@link UrlRequest}. May be null if the data source is not opened. */ @UnstableApi @Nullable @@ -848,11 +859,26 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { return responseInfo; } + // The nullness checker can't prove that UrlRequest.Builder.build() doesn't null out + // this.currentUrlRequestCallback. + // TODO: Add @SideEffectFree to the UrlRequest.Builder.build() stub + @SuppressWarnings("nullness:contracts.postcondition.not.satisfied") + @EnsuresNonNull({"this.currentUrlRequestCallback", "this.currentUrlRequest"}) + private void createCurrentUrlRequestAndCallback(DataSpec dataSpec) throws IOException { + currentUrlRequestCallback = new UrlRequestCallback(); + currentUrlRequest = buildRequestBuilder(dataSpec).build(); + } + + /** + * Returns {@link UrlRequest.Builder} from dataSpec. Would not work if data source is not opened. + */ @UnstableApi + @SideEffectFree + @RequiresNonNull("this.currentUrlRequestCallback") protected UrlRequest.Builder buildRequestBuilder(DataSpec dataSpec) throws IOException { UrlRequest.Builder requestBuilder = cronetEngine - .newUrlRequestBuilder(dataSpec.uri.toString(), urlRequestCallback, executor) + .newUrlRequestBuilder(dataSpec.uri.toString(), currentUrlRequestCallback, executor) .setPriority(requestPriority) .allowDirectExecutor(); @@ -1068,13 +1094,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { return TextUtils.join(";", setCookieHeaders); } - private static void attachCookies(UrlRequest.Builder requestBuilder, @Nullable String cookies) { - if (TextUtils.isEmpty(cookies)) { - return; - } - requestBuilder.addHeader(HttpHeaders.COOKIE, cookies); - } - private static int getStatus(UrlRequest request) throws InterruptedException { final ConditionVariable conditionVariable = new ConditionVariable(); final int[] statusHolder = new int[1]; @@ -1107,15 +1126,23 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { return remaining; } - private final class UrlRequestCallback extends UrlRequest.Callback { + @VisibleForTesting + /* package */ final class UrlRequestCallback extends UrlRequest.Callback { + + private volatile boolean isClosed = false; + + public void close() { + this.isClosed = true; + } @Override public synchronized void onRedirectReceived( UrlRequest request, UrlResponseInfo info, String newLocationUrl) { - if (request != currentUrlRequest) { + if (isClosed) { return; } - UrlRequest urlRequest = Assertions.checkNotNull(currentUrlRequest); + Assertions.checkNotNull(currentUrlRequest); + Assertions.checkNotNull(currentUrlRequestCallback); DataSpec dataSpec = Assertions.checkNotNull(currentDataSpec); int responseCode = info.getHttpStatusCode(); if (dataSpec.httpMethod == DataSpec.HTTP_METHOD_POST) { @@ -1156,7 +1183,6 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { return; } - urlRequest.cancel(); DataSpec redirectUrlDataSpec; if (!shouldKeepPost && dataSpec.httpMethod == DataSpec.HTTP_METHOD_POST) { // For POST redirects that aren't 307 or 308, the redirect is followed but request is @@ -1171,21 +1197,29 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { } else { redirectUrlDataSpec = dataSpec.withUri(Uri.parse(newLocationUrl)); } - UrlRequest.Builder requestBuilder; + + if (!TextUtils.isEmpty(cookieHeadersValue)) { + Map requestHeaders = new HashMap<>(); + requestHeaders.putAll(dataSpec.httpRequestHeaders); + requestHeaders.put(HttpHeaders.COOKIE, cookieHeadersValue); + redirectUrlDataSpec = + redirectUrlDataSpec.buildUpon().setHttpRequestHeaders(requestHeaders).build(); + } + + closeCurrentUrlRequestAndCallback(); try { - requestBuilder = buildRequestBuilder(redirectUrlDataSpec); + createCurrentUrlRequestAndCallback(redirectUrlDataSpec); } catch (IOException e) { exception = e; return; } - attachCookies(requestBuilder, cookieHeadersValue); - currentUrlRequest = requestBuilder.build(); + currentUrlRequest.start(); } @Override public synchronized void onResponseStarted(UrlRequest request, UrlResponseInfo info) { - if (request != currentUrlRequest) { + if (isClosed) { return; } responseInfo = info; @@ -1195,7 +1229,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { @Override public synchronized void onReadCompleted( UrlRequest request, UrlResponseInfo info, ByteBuffer buffer) { - if (request != currentUrlRequest) { + if (isClosed) { return; } operation.open(); @@ -1203,7 +1237,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { @Override public synchronized void onSucceeded(UrlRequest request, UrlResponseInfo info) { - if (request != currentUrlRequest) { + if (isClosed) { return; } finished = true; @@ -1213,7 +1247,7 @@ public class CronetDataSource extends BaseDataSource implements HttpDataSource { @Override public synchronized void onFailed( UrlRequest request, UrlResponseInfo info, CronetException error) { - if (request != currentUrlRequest) { + if (isClosed) { return; } if (error instanceof NetworkException diff --git a/libraries/datasource_cronet/src/test/java/androidx/media3/datasource/cronet/CronetDataSourceTest.java b/libraries/datasource_cronet/src/test/java/androidx/media3/datasource/cronet/CronetDataSourceTest.java index 3cc4161f08..143d3076e9 100644 --- a/libraries/datasource_cronet/src/test/java/androidx/media3/datasource/cronet/CronetDataSourceTest.java +++ b/libraries/datasource_cronet/src/test/java/androidx/media3/datasource/cronet/CronetDataSourceTest.java @@ -43,6 +43,7 @@ import androidx.media3.datasource.HttpDataSource; import androidx.media3.datasource.HttpDataSource.HttpDataSourceException; import androidx.media3.datasource.HttpDataSource.InvalidResponseCodeException; import androidx.media3.datasource.TransferListener; +import androidx.media3.datasource.cronet.CronetDataSource.UrlRequestCallback; import androidx.test.ext.junit.runners.AndroidJUnit4; import java.io.IOException; import java.io.InterruptedIOException; @@ -234,6 +235,7 @@ public final class CronetDataSourceTest { mockResponseStartSuccess(); dataSourceUnderTest.open(testDataSpec); + UrlRequestCallback previousRequestCallback = dataSourceUnderTest.currentUrlRequestCallback; dataSourceUnderTest.close(); // Prepare a mock UrlRequest to be used in the second open() call. final UrlRequest mockUrlRequest2 = mock(UrlRequest.class); @@ -241,13 +243,13 @@ public final class CronetDataSourceTest { doAnswer( invocation -> { // Invoke the callback for the previous request. - dataSourceUnderTest.urlRequestCallback.onFailed( + previousRequestCallback.onFailed( mockUrlRequest, testUrlResponseInfo, createNetworkException( /* errorCode= */ Integer.MAX_VALUE, /* cause= */ new IllegalArgumentException())); - dataSourceUnderTest.urlRequestCallback.onResponseStarted( + dataSourceUnderTest.currentUrlRequestCallback.onResponseStarted( mockUrlRequest2, testUrlResponseInfo); return null; }) @@ -1094,7 +1096,8 @@ public final class CronetDataSourceTest { /* nowMs= */ startTimeMs + TEST_CONNECT_TIMEOUT_MS - 1); assertNotCountedDown(openLatch); // The response arrives just in time. - dataSourceUnderTest.urlRequestCallback.onResponseStarted(mockUrlRequest, testUrlResponseInfo); + dataSourceUnderTest.currentUrlRequestCallback.onResponseStarted( + mockUrlRequest, testUrlResponseInfo); openLatch.await(); assertThat(exceptionOnTestThread.get()).isNull(); } @@ -1130,7 +1133,7 @@ public final class CronetDataSourceTest { /* nowMs= */ startTimeMs + TEST_CONNECT_TIMEOUT_MS - 1); assertNotCountedDown(timedOutLatch); // A redirect arrives just in time. - dataSourceUnderTest.urlRequestCallback.onRedirectReceived( + dataSourceUnderTest.currentUrlRequestCallback.onRedirectReceived( mockUrlRequest, testUrlResponseInfo, "RandomRedirectedUrl1"); long newTimeoutMs = 2 * TEST_CONNECT_TIMEOUT_MS - 1; @@ -1138,7 +1141,7 @@ public final class CronetDataSourceTest { // We should still be trying to open as we approach the new timeout. assertNotCountedDown(timedOutLatch); // A redirect arrives just in time. - dataSourceUnderTest.urlRequestCallback.onRedirectReceived( + dataSourceUnderTest.currentUrlRequestCallback.onRedirectReceived( mockUrlRequest, testUrlResponseInfo, "RandomRedirectedUrl2"); newTimeoutMs = 3 * TEST_CONNECT_TIMEOUT_MS - 2; @@ -1223,6 +1226,30 @@ public final class CronetDataSourceTest { verify(mockUrlRequest, times(2)).start(); } + @Test + public void testRedirect_dataSourceHandlesSetCookie_previousRequestIsCanceled() + throws HttpDataSourceException { + dataSourceUnderTest = + (CronetDataSource) + new CronetDataSource.Factory(mockCronetEngine, executorService) + .setConnectionTimeoutMs(TEST_CONNECT_TIMEOUT_MS) + .setReadTimeoutMs(TEST_READ_TIMEOUT_MS) + .setResetTimeoutOnRedirects(true) + .setHandleSetCookieRequests(true) + .createDataSource(); + dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE); + + mockSingleRedirectSuccess(/* responseCode= */ 300); + + testResponseHeader.put("Set-Cookie", "testcookie=testcookie; Path=/video"); + + dataSourceUnderTest.open(testDataSpec); + + verify(mockUrlRequest, never()).followRedirect(); + verify(mockUrlRequest).cancel(); + verify(mockUrlRequest, times(2)).start(); + } + @Test public void redirectNoSetCookieFollowsRedirect() throws HttpDataSourceException { mockSingleRedirectSuccess(/* responseCode= */ 300); @@ -1560,7 +1587,7 @@ public final class CronetDataSourceTest { private void mockResponseStartSuccess() { doAnswer( invocation -> { - dataSourceUnderTest.urlRequestCallback.onResponseStarted( + dataSourceUnderTest.currentUrlRequestCallback.onResponseStarted( mockUrlRequest, testUrlResponseInfo); return null; }) @@ -1571,7 +1598,7 @@ public final class CronetDataSourceTest { private void mockResponseStartRedirect() { doAnswer( invocation -> { - dataSourceUnderTest.urlRequestCallback.onRedirectReceived( + dataSourceUnderTest.currentUrlRequestCallback.onRedirectReceived( mockUrlRequest, createUrlResponseInfo(307), // statusCode "http://redirect.location.com"); @@ -1586,12 +1613,12 @@ public final class CronetDataSourceTest { invocation -> { if (!redirectCalled) { redirectCalled = true; - dataSourceUnderTest.urlRequestCallback.onRedirectReceived( + dataSourceUnderTest.currentUrlRequestCallback.onRedirectReceived( mockUrlRequest, createUrlResponseInfoWithUrl("http://example.com/video", responseCode), "http://example.com/video/redirect"); } else { - dataSourceUnderTest.urlRequestCallback.onResponseStarted( + dataSourceUnderTest.currentUrlRequestCallback.onResponseStarted( mockUrlRequest, testUrlResponseInfo); } return null; @@ -1603,7 +1630,7 @@ public final class CronetDataSourceTest { private void mockFollowRedirectSuccess() { doAnswer( invocation -> { - dataSourceUnderTest.urlRequestCallback.onResponseStarted( + dataSourceUnderTest.currentUrlRequestCallback.onResponseStarted( mockUrlRequest, testUrlResponseInfo); return null; }) @@ -1614,7 +1641,7 @@ public final class CronetDataSourceTest { private void mockResponseStartFailure(int errorCode, Throwable cause) { doAnswer( invocation -> { - dataSourceUnderTest.urlRequestCallback.onFailed( + dataSourceUnderTest.currentUrlRequestCallback.onFailed( mockUrlRequest, createUrlResponseInfo(500), // statusCode createNetworkException(errorCode, cause)); @@ -1629,7 +1656,7 @@ public final class CronetDataSourceTest { doAnswer( invocation -> { if (positionAndRemaining[1] == 0) { - dataSourceUnderTest.urlRequestCallback.onSucceeded( + dataSourceUnderTest.currentUrlRequestCallback.onSucceeded( mockUrlRequest, testUrlResponseInfo); } else { ByteBuffer inputBuffer = (ByteBuffer) invocation.getArguments()[0]; @@ -1637,7 +1664,7 @@ public final class CronetDataSourceTest { inputBuffer.put(buildTestDataBuffer(positionAndRemaining[0], readLength)); positionAndRemaining[0] += readLength; positionAndRemaining[1] -= readLength; - dataSourceUnderTest.urlRequestCallback.onReadCompleted( + dataSourceUnderTest.currentUrlRequestCallback.onReadCompleted( mockUrlRequest, testUrlResponseInfo, inputBuffer); } return null; @@ -1649,7 +1676,7 @@ public final class CronetDataSourceTest { private void mockReadFailure() { doAnswer( invocation -> { - dataSourceUnderTest.urlRequestCallback.onFailed( + dataSourceUnderTest.currentUrlRequestCallback.onFailed( mockUrlRequest, createUrlResponseInfo(500), // statusCode createNetworkException(