Skip to content

SDK / exoplayer

Go to a project
Commit 907b9bf4 by olly Committed by Oliver Woodman
Options

Sanitize threading in CronetDataSource 

- Move nearly all logic onto the calling thread (i.e. the thread
  calling open/read/close), to make threading correctness more
  obvious.
- Document which variables are read/written from which thread, and
  why the call sequences are safe.
- Fix thread safety issue that I think could probably cause data
  corruption in the case of a read timeout followed by another
  request into the DataSource.

Also:

- Relaxed content length checking to be consistent with the other
  http DataSource implementations, and avoided parsing the headers
  where they're not used.
- Fixed missing generics in CronetDataSourceFactory.
- Added TODO to work with servers that don't support partial range
  requests.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=135702217
parent 4fab4022
Expand all
Inline Side-by-side
Showing with 16 additions and 58 deletions
extensions/cronet/src/androidTest/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceTest.java
...@@ -174,10 +174,7 @@ public final class CronetDataSourceTest { ...@@ -174,10 +174,7 @@ public final class CronetDataSourceTest {
@Test(expected = IllegalStateException.class) @Test(expected = IllegalStateException.class)
public void testOpeningTwiceThrows() throws HttpDataSourceException { public void testOpeningTwiceThrows() throws HttpDataSourceException {
mockResponseStartSuccess(); mockResponseStartSuccess();
assertConnectionState(CronetDataSource.IDLE_CONNECTION);
dataSourceUnderTest.open(testDataSpec); dataSourceUnderTest.open(testDataSpec);
assertConnectionState(CronetDataSource.OPEN_CONNECTION);
dataSourceUnderTest.open(testDataSpec); dataSourceUnderTest.open(testDataSpec);
} }
...@@ -205,7 +202,7 @@ public final class CronetDataSourceTest { ...@@ -205,7 +202,7 @@ public final class CronetDataSourceTest {
dataSourceUnderTest.onFailed( dataSourceUnderTest.onFailed(
mockUrlRequest, mockUrlRequest,
testUrlResponseInfo, testUrlResponseInfo,
null); mockUrlRequestException);
dataSourceUnderTest.onResponseStarted( dataSourceUnderTest.onResponseStarted(
mockUrlRequest2, mockUrlRequest2,
testUrlResponseInfo); testUrlResponseInfo);
...@@ -253,13 +250,10 @@ public final class CronetDataSourceTest { ...@@ -253,13 +250,10 @@ public final class CronetDataSourceTest {
@Test @Test
public void testRequestOpen() throws HttpDataSourceException { public void testRequestOpen() throws HttpDataSourceException {
mockResponseStartSuccess(); mockResponseStartSuccess();
assertEquals(TEST_CONTENT_LENGTH, dataSourceUnderTest.open(testDataSpec)); assertEquals(TEST_CONTENT_LENGTH, dataSourceUnderTest.open(testDataSpec));
assertConnectionState(CronetDataSource.OPEN_CONNECTION);
verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testDataSpec); verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testDataSpec);
} }
@Test @Test
public void testRequestOpenGzippedCompressedReturnsDataSpecLength() public void testRequestOpenGzippedCompressedReturnsDataSpecLength()
throws HttpDataSourceException { throws HttpDataSourceException {
...@@ -271,7 +265,6 @@ public final class CronetDataSourceTest { ...@@ -271,7 +265,6 @@ public final class CronetDataSourceTest {
testDataSpec = new DataSpec(Uri.parse(TEST_URL), 1000, 5000, null); testDataSpec = new DataSpec(Uri.parse(TEST_URL), 1000, 5000, null);
assertEquals(5000 /* contentLength */, dataSourceUnderTest.open(testDataSpec)); assertEquals(5000 /* contentLength */, dataSourceUnderTest.open(testDataSpec));
assertConnectionState(CronetDataSource.OPEN_CONNECTION);
verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testDataSpec); verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testDataSpec);
} }
...@@ -286,7 +279,6 @@ public final class CronetDataSourceTest { ...@@ -286,7 +279,6 @@ public final class CronetDataSourceTest {
// Check for connection not automatically closed. // Check for connection not automatically closed.
assertFalse(e.getCause() instanceof UnknownHostException); assertFalse(e.getCause() instanceof UnknownHostException);
verify(mockUrlRequest, never()).cancel(); verify(mockUrlRequest, never()).cancel();
assertConnectionState(CronetDataSource.OPENING_CONNECTION);
verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec);
} }
} }
...@@ -304,7 +296,6 @@ public final class CronetDataSourceTest { ...@@ -304,7 +296,6 @@ public final class CronetDataSourceTest {
// Check for connection not automatically closed. // Check for connection not automatically closed.
assertTrue(e.getCause() instanceof UnknownHostException); assertTrue(e.getCause() instanceof UnknownHostException);
verify(mockUrlRequest, never()).cancel(); verify(mockUrlRequest, never()).cancel();
assertConnectionState(CronetDataSource.OPENING_CONNECTION);
verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec);
} }
} }
...@@ -321,7 +312,6 @@ public final class CronetDataSourceTest { ...@@ -321,7 +312,6 @@ public final class CronetDataSourceTest {
assertTrue(e instanceof HttpDataSource.InvalidResponseCodeException); assertTrue(e instanceof HttpDataSource.InvalidResponseCodeException);
// Check for connection not automatically closed. // Check for connection not automatically closed.
verify(mockUrlRequest, never()).cancel(); verify(mockUrlRequest, never()).cancel();
assertConnectionState(CronetDataSource.OPENING_CONNECTION);
verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec);
} }
} }
...@@ -338,37 +328,16 @@ public final class CronetDataSourceTest { ...@@ -338,37 +328,16 @@ public final class CronetDataSourceTest {
assertTrue(e instanceof HttpDataSource.InvalidContentTypeException); assertTrue(e instanceof HttpDataSource.InvalidContentTypeException);
// Check for connection not automatically closed. // Check for connection not automatically closed.
verify(mockUrlRequest, never()).cancel(); verify(mockUrlRequest, never()).cancel();
assertConnectionState(CronetDataSource.OPENING_CONNECTION);
verify(mockContentTypePredicate).evaluate(TEST_CONTENT_TYPE); verify(mockContentTypePredicate).evaluate(TEST_CONTENT_TYPE);
} }
} }
@Test @Test
public void testRequestOpenValidatesContentLength() {
mockResponseStartSuccess();
// Data spec's requested length, 5000. Test response's length, 16,000.
testDataSpec = new DataSpec(Uri.parse(TEST_URL), 1000, 5000, null);
try {
dataSourceUnderTest.open(testDataSpec);
fail("HttpDataSource.HttpDataSourceException expected");
} catch (HttpDataSourceException e) {
verify(mockUrlRequest).addHeader("Range", "bytes=1000-5999");
// Check for connection not automatically closed.
verify(mockUrlRequest, never()).cancel();
assertConnectionState(CronetDataSource.OPENING_CONNECTION);
verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testPostDataSpec);
}
}
@Test
public void testPostRequestOpen() throws HttpDataSourceException { public void testPostRequestOpen() throws HttpDataSourceException {
mockResponseStartSuccess(); mockResponseStartSuccess();
dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE); dataSourceUnderTest.setRequestProperty("Content-Type", TEST_CONTENT_TYPE);
assertEquals(TEST_CONTENT_LENGTH, dataSourceUnderTest.open(testPostDataSpec)); assertEquals(TEST_CONTENT_LENGTH, dataSourceUnderTest.open(testPostDataSpec));
assertConnectionState(CronetDataSource.OPEN_CONNECTION);
verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testPostDataSpec); verify(mockTransferListener).onTransferStart(dataSourceUnderTest, testPostDataSpec);
} }
...@@ -510,7 +479,6 @@ public final class CronetDataSourceTest { ...@@ -510,7 +479,6 @@ public final class CronetDataSourceTest {
dataSourceUnderTest.close(); dataSourceUnderTest.close();
verify(mockTransferListener).onTransferEnd(dataSourceUnderTest); verify(mockTransferListener).onTransferEnd(dataSourceUnderTest);
assertConnectionState(CronetDataSource.IDLE_CONNECTION);
try { try {
bytesRead += dataSourceUnderTest.read(returnedBuffer, 0, 8); bytesRead += dataSourceUnderTest.read(returnedBuffer, 0, 8);
...@@ -572,7 +540,6 @@ public final class CronetDataSourceTest { ...@@ -572,7 +540,6 @@ public final class CronetDataSourceTest {
verify(mockUrlRequest, times(1)).read(any(ByteBuffer.class)); verify(mockUrlRequest, times(1)).read(any(ByteBuffer.class));
// Check for connection not automatically closed. // Check for connection not automatically closed.
verify(mockUrlRequest, never()).cancel(); verify(mockUrlRequest, never()).cancel();
assertConnectionState(CronetDataSource.OPEN_CONNECTION);
assertEquals(16, bytesRead); assertEquals(16, bytesRead);
} }
...@@ -603,15 +570,12 @@ public final class CronetDataSourceTest { ...@@ -603,15 +570,12 @@ public final class CronetDataSourceTest {
// We should still be trying to open. // We should still be trying to open.
assertFalse(timedOutCondition.block(50)); assertFalse(timedOutCondition.block(50));
assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState);
// We should still be trying to open as we approach the timeout. // We should still be trying to open as we approach the timeout.
when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS - 1); when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS - 1);
assertFalse(timedOutCondition.block(50)); assertFalse(timedOutCondition.block(50));
assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState);
// Now we timeout. // Now we timeout.
when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS); when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS);
timedOutCondition.block(); timedOutCondition.block();
assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState);
verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec);
} }
...@@ -637,15 +601,12 @@ public final class CronetDataSourceTest { ...@@ -637,15 +601,12 @@ public final class CronetDataSourceTest {
// We should still be trying to open. // We should still be trying to open.
assertFalse(openCondition.block(50)); assertFalse(openCondition.block(50));
assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState);
// We should still be trying to open as we approach the timeout. // We should still be trying to open as we approach the timeout.
when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS - 1); when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS - 1);
assertFalse(openCondition.block(50)); assertFalse(openCondition.block(50));
assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState);
// The response arrives just in time. // The response arrives just in time.
dataSourceUnderTest.onResponseStarted(mockUrlRequest, testUrlResponseInfo); dataSourceUnderTest.onResponseStarted(mockUrlRequest, testUrlResponseInfo);
openCondition.block(); openCondition.block();
assertEquals(CronetDataSource.OPEN_CONNECTION, dataSourceUnderTest.connectionState);
} }
@Test @Test
...@@ -674,11 +635,9 @@ public final class CronetDataSourceTest { ...@@ -674,11 +635,9 @@ public final class CronetDataSourceTest {
// We should still be trying to open. // We should still be trying to open.
assertFalse(timedOutCondition.block(50)); assertFalse(timedOutCondition.block(50));
assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState);
// We should still be trying to open as we approach the timeout. // We should still be trying to open as we approach the timeout.
when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS - 1); when(mockClock.elapsedRealtime()).thenReturn((long) TEST_CONNECT_TIMEOUT_MS - 1);
assertFalse(timedOutCondition.block(50)); assertFalse(timedOutCondition.block(50));
assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState);
// A redirect arrives just in time. // A redirect arrives just in time.
dataSourceUnderTest.onRedirectReceived(mockUrlRequest, testUrlResponseInfo, dataSourceUnderTest.onRedirectReceived(mockUrlRequest, testUrlResponseInfo,
"RandomRedirectedUrl1"); "RandomRedirectedUrl1");
...@@ -689,7 +648,6 @@ public final class CronetDataSourceTest { ...@@ -689,7 +648,6 @@ public final class CronetDataSourceTest {
assertFalse(timedOutCondition.block(newTimeoutMs)); assertFalse(timedOutCondition.block(newTimeoutMs));
// We should still be trying to open as we approach the new timeout. // We should still be trying to open as we approach the new timeout.
assertFalse(timedOutCondition.block(50)); assertFalse(timedOutCondition.block(50));
assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState);
// A redirect arrives just in time. // A redirect arrives just in time.
dataSourceUnderTest.onRedirectReceived(mockUrlRequest, testUrlResponseInfo, dataSourceUnderTest.onRedirectReceived(mockUrlRequest, testUrlResponseInfo,
"RandomRedirectedUrl2"); "RandomRedirectedUrl2");
...@@ -700,11 +658,9 @@ public final class CronetDataSourceTest { ...@@ -700,11 +658,9 @@ public final class CronetDataSourceTest {
assertFalse(timedOutCondition.block(newTimeoutMs)); assertFalse(timedOutCondition.block(newTimeoutMs));
// We should still be trying to open as we approach the new timeout. // We should still be trying to open as we approach the new timeout.
assertFalse(timedOutCondition.block(50)); assertFalse(timedOutCondition.block(50));
assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState);
// Now we timeout. // Now we timeout.
when(mockClock.elapsedRealtime()).thenReturn(newTimeoutMs); when(mockClock.elapsedRealtime()).thenReturn(newTimeoutMs);
timedOutCondition.block(); timedOutCondition.block();
assertEquals(CronetDataSource.OPENING_CONNECTION, dataSourceUnderTest.connectionState);
verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec); verify(mockTransferListener, never()).onTransferStart(dataSourceUnderTest, testDataSpec);
assertEquals(1, openExceptions.get()); assertEquals(1, openExceptions.get());
...@@ -818,7 +774,7 @@ public final class CronetDataSourceTest { ...@@ -818,7 +774,7 @@ public final class CronetDataSourceTest {
dataSourceUnderTest.onFailed( dataSourceUnderTest.onFailed(
mockUrlRequest, mockUrlRequest,
createUrlResponseInfo(500), // statusCode createUrlResponseInfo(500), // statusCode
null); mockUrlRequestException);
return null; return null;
} }
}).when(mockUrlRequest).read(any(ByteBuffer.class)); }).when(mockUrlRequest).read(any(ByteBuffer.class));
...@@ -869,8 +825,4 @@ public final class CronetDataSourceTest { ...@@ -869,8 +825,4 @@ public final class CronetDataSourceTest {
return testBuffer; return testBuffer;
} }
private void assertConnectionState(int state) {
assertEquals(state, dataSourceUnderTest.connectionState);
}
} }
extensions/cronet/src/main/java/com/google/android/exoplayer2/ext/cronet/CronetDataSourceFactory.java
...@@ -41,7 +41,7 @@ public final class CronetDataSourceFactory implements Factory { ...@@ -41,7 +41,7 @@ public final class CronetDataSourceFactory implements Factory {
private final CronetEngine cronetEngine; private final CronetEngine cronetEngine;
private final Executor executor; private final Executor executor;
private final Predicate<String> contentTypePredicate; private final Predicate<String> contentTypePredicate;
private final TransferListener transferListener; private final TransferListener<? super DataSource> transferListener;
private final int connectTimeoutMs; private final int connectTimeoutMs;
private final int readTimeoutMs; private final int readTimeoutMs;
private final boolean resetTimeoutOnRedirects; private final boolean resetTimeoutOnRedirects;
......
extensions/okhttp/src/main/java/com/google/android/exoplayer2/ext/okhttp/OkHttpDataSource.java
...@@ -185,9 +185,12 @@ public class OkHttpDataSource implements HttpDataSource { ...@@ -185,9 +185,12 @@ public class OkHttpDataSource implements HttpDataSource {
bytesToSkip = responseCode == 200 && dataSpec.position != 0 ? dataSpec.position : 0; bytesToSkip = responseCode == 200 && dataSpec.position != 0 ? dataSpec.position : 0;
// Determine the length of the data to be read, after skipping. // Determine the length of the data to be read, after skipping.
long contentLength = response.body().contentLength(); if (dataSpec.length != C.LENGTH_UNSET) {
bytesToRead = dataSpec.length != C.LENGTH_UNSET ? dataSpec.length bytesToRead = dataSpec.length;
: (contentLength != -1 ? (contentLength - bytesToSkip) : C.LENGTH_UNSET); } else {
long contentLength = response.body().contentLength();
bytesToRead = contentLength != -1 ? (contentLength - bytesToSkip) : C.LENGTH_UNSET;
}
opened = true; opened = true;
if (listener != null) { if (listener != null) {
......
library/src/main/java/com/google/android/exoplayer2/upstream/DefaultHttpDataSource.java
...@@ -231,10 +231,13 @@ public class DefaultHttpDataSource implements HttpDataSource { ...@@ -231,10 +231,13 @@ public class DefaultHttpDataSource implements HttpDataSource {
// Determine the length of the data to be read, after skipping. // Determine the length of the data to be read, after skipping.
if ((dataSpec.flags & DataSpec.FLAG_ALLOW_GZIP) == 0) { if ((dataSpec.flags & DataSpec.FLAG_ALLOW_GZIP) == 0) {
long contentLength = getContentLength(connection); if (dataSpec.length != C.LENGTH_UNSET) {
bytesToRead = dataSpec.length != C.LENGTH_UNSET ? dataSpec.length bytesToRead = dataSpec.length;
: contentLength != C.LENGTH_UNSET ? contentLength - bytesToSkip } else {
: C.LENGTH_UNSET; long contentLength = getContentLength(connection);
bytesToRead = contentLength != C.LENGTH_UNSET ? (contentLength - bytesToSkip)
: C.LENGTH_UNSET;
}
} else { } else {
// Gzip is enabled. If the server opts to use gzip then the content length in the response // Gzip is enabled. If the server opts to use gzip then the content length in the response
// will be that of the compressed data, which isn't what we want. Furthermore, there isn't a // will be that of the compressed data, which isn't what we want. Furthermore, there isn't a
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or sign in to comment