Skip to content

Commit

Permalink
Tighten DataSource contract test assertions
Browse files Browse the repository at this point in the history 
Assert that opening the DataSource at the end of the resource
results in only RESULT_END_OF_INPUT being read.

open() and read() are still permitted to throw, although this
permissiveness will be removed in subsequent commits.

PiperOrigin-RevId: 362905314
  • Loading branch information
@ojw28
ojw28 committed Mar 15, 2021
1 parent 6f8a8fb commit 10de7b2
Show file tree
Hide file tree
Showing 9 changed files with 144 additions and 93 deletions.
59 changes: 36 additions & 23 deletions library/core/src/main/java/com/google/android/exoplayer2/upstream/ContentDataSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,48 +72,61 @@ public long open(DataSpec dataSpec) throws ContentDataSourceException {
if (assetFileDescriptor == null) {
throw new FileNotFoundException("Could not open file descriptor for: " + uri);
}

long assetFileDescriptorLength = assetFileDescriptor.getLength();
FileInputStream inputStream = new FileInputStream(assetFileDescriptor.getFileDescriptor());
this.inputStream = inputStream;

long assetStartOffset = assetFileDescriptor.getStartOffset();
long skipped = inputStream.skip(assetStartOffset + dataSpec.position) - assetStartOffset;
// We can't rely only on the "skipped < dataSpec.position" check below to detect whether the
// position is beyond the end of the asset being read. This is because the file may contain
// multiple assets, and there's nothing to prevent InputStream.skip() from succeeding by
// skipping into the data of the next asset. Hence we also need to check against the asset
// length explicitly, which is guaranteed to be set unless the asset extends to the end of the
// file.
if (assetFileDescriptorLength != AssetFileDescriptor.UNKNOWN_LENGTH
&& dataSpec.position > assetFileDescriptorLength) {
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
long assetFileDescriptorOffset = assetFileDescriptor.getStartOffset();
long skipped =
inputStream.skip(assetFileDescriptorOffset + dataSpec.position)
- assetFileDescriptorOffset;
if (skipped != dataSpec.position) {
// We expect the skip to be satisfied in full. If it isn't then we're probably trying to
// skip beyond the end of the data.
// read beyond the end of the last resource in the file.
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
if (dataSpec.length != C.LENGTH_UNSET) {
bytesRemaining = dataSpec.length;
} else {
long assetFileDescriptorLength = assetFileDescriptor.getLength();
if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) {
// The asset must extend to the end of the file. If FileInputStream.getChannel().size()
// returns 0 then the remaining length cannot be determined.
FileChannel channel = inputStream.getChannel();
long channelSize = channel.size();
if (channelSize == 0) {
bytesRemaining = C.LENGTH_UNSET;
} else {
bytesRemaining = channelSize - channel.position();
if (bytesRemaining < 0) {
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
}
if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) {
// The asset must extend to the end of the file. We can try and resolve the length with
// FileInputStream.getChannel().size().
FileChannel channel = inputStream.getChannel();
long channelSize = channel.size();
if (channelSize == 0) {
bytesRemaining = C.LENGTH_UNSET;
} else {
bytesRemaining = assetFileDescriptorLength - skipped;
bytesRemaining = channelSize - channel.position();
if (bytesRemaining < 0) {
// The skip above was satisfied in full, but skipped beyond the end of the file.
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
}
} else {
bytesRemaining = assetFileDescriptorLength - skipped;
if (bytesRemaining < 0) {
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
}
} catch (IOException e) {
throw new ContentDataSourceException(e);
}

if (dataSpec.length != C.LENGTH_UNSET) {
bytesRemaining =
bytesRemaining == C.LENGTH_UNSET ? dataSpec.length : min(bytesRemaining, dataSpec.length);
}
opened = true;
transferStarted(dataSpec);

return bytesRemaining;
return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : bytesRemaining;
}

@Override
Expand Down
40 changes: 28 additions & 12 deletions library/core/src/main/java/com/google/android/exoplayer2/upstream/RawResourceDataSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.channels.FileChannel;

/**
* A {@link DataSource} for reading a raw resource inside the APK.
Expand Down Expand Up @@ -149,6 +150,7 @@ public long open(DataSpec dataSpec) throws RawResourceDataSourceException {
long assetFileDescriptorLength = assetFileDescriptor.getLength();
FileInputStream inputStream = new FileInputStream(assetFileDescriptor.getFileDescriptor());
this.inputStream = inputStream;

try {
// We can't rely only on the "skipped < dataSpec.position" check below to detect whether the
// position is beyond the end of the resource being read. This is because the file will
Expand All @@ -160,31 +162,45 @@ public long open(DataSpec dataSpec) throws RawResourceDataSourceException {
&& dataSpec.position > assetFileDescriptorLength) {
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
inputStream.skip(assetFileDescriptor.getStartOffset());
long skipped = inputStream.skip(dataSpec.position);
if (skipped < dataSpec.position) {
long assetFileDescriptorOffset = assetFileDescriptor.getStartOffset();
long skipped =
inputStream.skip(assetFileDescriptorOffset + dataSpec.position)
- assetFileDescriptorOffset;
if (skipped != dataSpec.position) {
// We expect the skip to be satisfied in full. If it isn't then we're probably trying to
// read beyond the end of the last resource in the file.
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
if (assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH) {
// The asset must extend to the end of the file. We can try and resolve the length with
// FileInputStream.getChannel().size().
FileChannel channel = inputStream.getChannel();
if (channel.size() == 0) {
bytesRemaining = C.LENGTH_UNSET;
} else {
bytesRemaining = channel.size() - channel.position();
if (bytesRemaining < 0) {
// The skip above was satisfied in full, but skipped beyond the end of the file.
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
}
} else {
bytesRemaining = assetFileDescriptorLength - skipped;
if (bytesRemaining < 0) {
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
}
} catch (IOException e) {
throw new RawResourceDataSourceException(e);
}

if (dataSpec.length != C.LENGTH_UNSET) {
bytesRemaining = dataSpec.length;
} else {
// If the length is UNKNOWN_LENGTH then the asset extends to the end of the file.
bytesRemaining =
assetFileDescriptorLength == AssetFileDescriptor.UNKNOWN_LENGTH
? C.LENGTH_UNSET
: (assetFileDescriptorLength - dataSpec.position);
bytesRemaining == C.LENGTH_UNSET ? dataSpec.length : min(bytesRemaining, dataSpec.length);
}

opened = true;
transferStarted(dataSpec);

return bytesRemaining;
return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : bytesRemaining;
}

@Override
Expand Down
48 changes: 37 additions & 11 deletions library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheDataSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -388,8 +388,9 @@ public interface EventListener {

@Nullable private Uri actualUri;
@Nullable private DataSpec requestDataSpec;
@Nullable private DataSpec currentDataSpec;
@Nullable private DataSource currentDataSource;
private boolean currentDataSpecLengthUnset;
private long currentDataSourceBytesRead;
private long readPosition;
private long bytesRemaining;
@Nullable private CacheSpan currentHoleSpan;
Expand Down Expand Up @@ -565,19 +566,27 @@ public long open(DataSpec dataSpec) throws IOException {
notifyCacheIgnored(reason);
}

if (dataSpec.length != C.LENGTH_UNSET || currentRequestIgnoresCache) {
bytesRemaining = dataSpec.length;
if (currentRequestIgnoresCache) {
bytesRemaining = C.LENGTH_UNSET;
} else {
bytesRemaining = ContentMetadata.getContentLength(cache.getContentMetadata(key));
if (bytesRemaining != C.LENGTH_UNSET) {
bytesRemaining -= dataSpec.position;
if (bytesRemaining <= 0) {
if (bytesRemaining < 0) {
throw new DataSourceException(DataSourceException.POSITION_OUT_OF_RANGE);
}
}
}
openNextSource(requestDataSpec, false);
return bytesRemaining;
if (dataSpec.length != C.LENGTH_UNSET) {
bytesRemaining =
bytesRemaining == C.LENGTH_UNSET
? dataSpec.length
: min(bytesRemaining, dataSpec.length);
}
if (bytesRemaining > 0 || bytesRemaining == C.LENGTH_UNSET) {
openNextSource(requestDataSpec, false);
}
return dataSpec.length != C.LENGTH_UNSET ? dataSpec.length : bytesRemaining;
} catch (Throwable e) {
handleBeforeThrow(e);
throw e;
Expand All @@ -587,6 +596,7 @@ public long open(DataSpec dataSpec) throws IOException {
@Override
public int read(byte[] buffer, int offset, int readLength) throws IOException {
DataSpec requestDataSpec = checkNotNull(this.requestDataSpec);
DataSpec currentDataSpec = checkNotNull(this.currentDataSpec);
if (readLength == 0) {
return 0;
}
Expand All @@ -603,10 +613,16 @@ public int read(byte[] buffer, int offset, int readLength) throws IOException {
totalCachedBytesRead += bytesRead;
}
readPosition += bytesRead;
currentDataSourceBytesRead += bytesRead;
if (bytesRemaining != C.LENGTH_UNSET) {
bytesRemaining -= bytesRead;
}
} else if (currentDataSpecLengthUnset) {
} else if (isReadingFromUpstream()
&& (currentDataSpec.length == C.LENGTH_UNSET
|| currentDataSourceBytesRead < currentDataSpec.length)) {
// We've encountered RESULT_END_OF_INPUT from the upstream DataSource at a position not
// imposed by the current DataSpec. This must mean that we've reached the end of the
// resource.
setNoBytesRemainingAndMaybeStoreLength(castNonNull(requestDataSpec.key));
} else if (bytesRemaining > 0 || bytesRemaining == C.LENGTH_UNSET) {
closeCurrentSource();
Expand All @@ -615,7 +631,16 @@ public int read(byte[] buffer, int offset, int readLength) throws IOException {
}
return bytesRead;
} catch (IOException e) {
if (currentDataSpecLengthUnset && DataSourceException.isCausedByPositionOutOfRange(e)) {
// TODO: This is not correct, because position-out-of-range exceptions should only be thrown
// if the requested position is more than one byte beyond the end of the resource. Conversely,
// this code is assuming that a position-out-of-range exception indicates the requested
// position is exactly one byte beyond the end of the resource, which is not a case for which
// this type of exception should be thrown. This exception handling may be required for
// interop with current HttpDataSource implementations that do (incorrectly) throw a
// position-out-of-range exception at this position. It should be removed when the
// HttpDataSource implementations are fixed.
if (currentDataSpec.length == C.LENGTH_UNSET
&& DataSourceException.isCausedByPositionOutOfRange(e)) {
setNoBytesRemainingAndMaybeStoreLength(castNonNull(requestDataSpec.key));
return C.RESULT_END_OF_INPUT;
}
Expand Down Expand Up @@ -760,12 +785,13 @@ private void openNextSource(DataSpec requestDataSpec, boolean checkCache) throws
currentHoleSpan = nextSpan;
}
currentDataSource = nextDataSource;
currentDataSpecLengthUnset = nextDataSpec.length == C.LENGTH_UNSET;
currentDataSpec = nextDataSpec;
currentDataSourceBytesRead = 0;
long resolvedLength = nextDataSource.open(nextDataSpec);

// Update bytesRemaining, actualUri and (if writing to cache) the cache metadata.
ContentMetadataMutations mutations = new ContentMetadataMutations();
if (currentDataSpecLengthUnset && resolvedLength != C.LENGTH_UNSET) {
if (nextDataSpec.length == C.LENGTH_UNSET && resolvedLength != C.LENGTH_UNSET) {
bytesRemaining = resolvedLength;
ContentMetadataMutations.setContentLength(mutations, readPosition + bytesRemaining);
}
Expand Down Expand Up @@ -816,8 +842,8 @@ private void closeCurrentSource() throws IOException {
try {
currentDataSource.close();
} finally {
currentDataSpec = null;
currentDataSource = null;
currentDataSpecLengthUnset = false;
if (currentHoleSpan != null) {
cache.releaseHoleSpan(currentHoleSpan);
currentHoleSpan = null;
Expand Down
9 changes: 9 additions & 0 deletions library/core/src/main/java/com/google/android/exoplayer2/upstream/cache/CacheWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.android.exoplayer2.util.PriorityTaskManager;
import com.google.android.exoplayer2.util.PriorityTaskManager.PriorityTooLowException;
import com.google.android.exoplayer2.util.Util;
import java.io.EOFException;
import java.io.IOException;
import java.io.InterruptedIOException;

Expand Down Expand Up @@ -142,6 +143,14 @@ public void cache() throws IOException {
nextPosition += readBlockToCache(nextPosition, nextRequestLength);
}
}

// TODO: Remove allowShortContent parameter, this code block, and return the number of bytes
// cached. The caller can then check whether fewer bytes were cached than were requested.
if (!allowShortContent
&& dataSpec.length != C.LENGTH_UNSET
&& nextPosition != dataSpec.position + dataSpec.length) {
throw new EOFException();
}
}

/**
Expand Down
6 changes: 3 additions & 3 deletions .../core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheDataSourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ public void unsatisfiableRange() throws Exception {
try {
cacheDataSource =
createCacheDataSource(/* setReadException= */ false, /* unknownLength= */ false);
cacheDataSource.open(buildDataSpec(TEST_DATA.length, /* length= */ 1, defaultCacheKey));
cacheDataSource.open(buildDataSpec(TEST_DATA.length + 1, /* length= */ 1, defaultCacheKey));
fail();
} catch (IOException e) {
// Expected.
Expand Down Expand Up @@ -204,7 +204,7 @@ public void unsatisfiableRangeWithCacheKeyFactoryNullDataSpecCacheKey() throws E
cacheDataSource =
createCacheDataSource(
/* setReadException= */ false, /* unknownLength= */ false, cacheKeyFactory);
cacheDataSource.open(buildDataSpec(TEST_DATA.length, /* length= */ 1, customCacheKey));
cacheDataSource.open(buildDataSpec(TEST_DATA.length + 1, /* length= */ 1, customCacheKey));
fail();
} catch (IOException e) {
// Expected.
Expand Down Expand Up @@ -257,7 +257,7 @@ public void unsatisfiableRangeWithCacheKeyFactoryOverridingDataSpecCacheKey() th
cacheDataSource =
createCacheDataSource(
/* setReadException= */ false, /* unknownLength= */ false, cacheKeyFactory);
cacheDataSource.open(buildDataSpec(TEST_DATA.length, /* length= */ 1, customCacheKey));
cacheDataSource.open(buildDataSpec(TEST_DATA.length + 1, /* length= */ 1, customCacheKey));
fail();
} catch (IOException e) {
// Expected.
Expand Down
24 changes: 9 additions & 15 deletions library/core/src/test/java/com/google/android/exoplayer2/upstream/cache/CacheWriterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,15 @@
import com.google.android.exoplayer2.testutil.FakeDataSet;
import com.google.android.exoplayer2.testutil.FakeDataSource;
import com.google.android.exoplayer2.testutil.TestUtil;
import com.google.android.exoplayer2.upstream.DataSourceException;
import com.google.android.exoplayer2.upstream.DataSpec;
import com.google.android.exoplayer2.upstream.FileDataSource;
import com.google.android.exoplayer2.util.Util;
import java.io.EOFException;
import java.io.File;
import java.io.IOException;
import java.util.concurrent.atomic.AtomicBoolean;
import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.MockitoAnnotations;
Expand Down Expand Up @@ -175,7 +174,6 @@ public void cacheUnknownLengthPartialCaching() throws Exception {
assertCachedData(cache, fakeDataSet);
}

@Ignore("Currently broken. See https://github.com/google/ExoPlayer/issues/7326.")
@Test
public void cacheLengthExceedsActualDataLength() throws Exception {
FakeDataSet fakeDataSet = new FakeDataSet().setRandomData("test_data", 100);
Expand Down Expand Up @@ -206,18 +204,14 @@ public void cacheThrowEOFException() throws Exception {
Uri testUri = Uri.parse("test_data");
DataSpec dataSpec = new DataSpec(testUri, /* position= */ 0, /* length= */ 1000);

IOException exception =
assertThrows(
IOException.class,
() ->
new CacheWriter(
new CacheDataSource(cache, dataSource),
dataSpec,
/* allowShortContent= */ false,
/* temporaryBuffer= */ null,
/* progressListener= */ null)
.cache());
assertThat(DataSourceException.isCausedByPositionOutOfRange(exception)).isTrue();
CacheWriter cacheWriter =
new CacheWriter(
new CacheDataSource(cache, dataSource),
dataSpec,
/* allowShortContent= */ false,
/* temporaryBuffer= */ null,
/* progressListener= */ null);
assertThrows(EOFException.class, cacheWriter::cache);
}

@Test
Expand Down
Loading

0 comments on commit 10de7b2

Please sign in to comment.