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(