diff --git a/RELEASENOTES.md b/RELEASENOTES.md index b01aafc4c7..fe7a2389da 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -27,6 +27,10 @@ * Session: * UI: * Downloads: + * Fix bug in `CacheWriter` that leaves data sources open and cache areas + locked in case the data source throws an `Exception` other than + `IOException` + ([#9760](https://github.com/google/ExoPlayer/issues/9760)). * OkHttp Extension: * Cronet Extension: * RTMP Extension: diff --git a/libraries/datasource/src/main/java/androidx/media3/datasource/cache/CacheWriter.java b/libraries/datasource/src/main/java/androidx/media3/datasource/cache/CacheWriter.java index b17bdbd59d..61a4aa3d99 100644 --- a/libraries/datasource/src/main/java/androidx/media3/datasource/cache/CacheWriter.java +++ b/libraries/datasource/src/main/java/androidx/media3/datasource/cache/CacheWriter.java @@ -159,7 +159,7 @@ public final class CacheWriter { try { resolvedLength = dataSource.open(boundedDataSpec); isDataSourceOpen = true; - } catch (IOException e) { + } catch (Exception e) { DataSourceUtil.closeQuietly(dataSource); } } @@ -172,7 +172,7 @@ public final class CacheWriter { dataSpec.buildUpon().setPosition(position).setLength(C.LENGTH_UNSET).build(); try { resolvedLength = dataSource.open(unboundedDataSpec); - } catch (IOException e) { + } catch (Exception e) { DataSourceUtil.closeQuietly(dataSource); throw e; } @@ -195,7 +195,7 @@ public final class CacheWriter { if (isLastBlock) { onRequestEndPosition(position + totalBytesRead); } - } catch (IOException e) { + } catch (Exception e) { DataSourceUtil.closeQuietly(dataSource); throw e; } diff --git a/libraries/datasource/src/test/java/androidx/media3/datasource/cache/CacheWriterTest.java b/libraries/datasource/src/test/java/androidx/media3/datasource/cache/CacheWriterTest.java index 2bdf548e04..13ffae53b0 100644 --- a/libraries/datasource/src/test/java/androidx/media3/datasource/cache/CacheWriterTest.java +++ b/libraries/datasource/src/test/java/androidx/media3/datasource/cache/CacheWriterTest.java @@ -22,8 +22,10 @@ import static org.junit.Assert.assertThrows; import android.net.Uri; import androidx.media3.common.C; import androidx.media3.common.util.Util; +import androidx.media3.datasource.DataSource; import androidx.media3.datasource.DataSpec; import androidx.media3.datasource.FileDataSource; +import androidx.media3.datasource.TransferListener; import androidx.media3.test.utils.FailOnCloseDataSink; import androidx.media3.test.utils.FakeDataSet; import androidx.media3.test.utils.FakeDataSource; @@ -263,6 +265,119 @@ public final class CacheWriterTest { assertCachedData(cache, fakeDataSet); } + @Test + public void cache_ioExceptionDuringOpen_closesDataSource() { + FakeDataSet fakeDataSet = new FakeDataSet().newData("test_data").appendReadData(1).endData(); + FakeDataSource dataSource = new FakeDataSource(fakeDataSet); + dataSource.addTransferListener( + new TransferListener() { + @Override + public void onTransferInitializing( + DataSource source, DataSpec dataSpec, boolean isNetwork) { + Util.sneakyThrow(new IOException()); + } + + @Override + public void onTransferStart(DataSource source, DataSpec dataSpec, boolean isNetwork) {} + + @Override + public void onBytesTransferred( + DataSource source, DataSpec dataSpec, boolean isNetwork, int bytesTransferred) {} + + @Override + public void onTransferEnd(DataSource source, DataSpec dataSpec, boolean isNetwork) {} + }); + CacheWriter cacheWriter = + new CacheWriter( + new CacheDataSource(cache, dataSource), + new DataSpec(Uri.parse("test_data")), + /* temporaryBuffer= */ null, + new CachingCounters()); + + assertThrows(IOException.class, cacheWriter::cache); + + assertThat(dataSource.isOpened()).isFalse(); + } + + @Test + public void cache_ioExceptionDuringRead_closesDataSource() { + FakeDataSet fakeDataSet = + new FakeDataSet() + .newData("test_data") + .appendReadError(new IOException()) + .appendReadData(1) + .endData(); + FakeDataSource dataSource = new FakeDataSource(fakeDataSet); + CacheWriter cacheWriter = + new CacheWriter( + new CacheDataSource(cache, dataSource), + new DataSpec(Uri.parse("test_data")), + /* temporaryBuffer= */ null, + new CachingCounters()); + + assertThrows(IOException.class, cacheWriter::cache); + + assertThat(dataSource.isOpened()).isFalse(); + } + + @Test + public void cache_nonIoExceptionDuringOpen_closesDataSource() { + FakeDataSet fakeDataSet = new FakeDataSet().newData("test_data").appendReadData(1).endData(); + FakeDataSource dataSource = new FakeDataSource(fakeDataSet); + dataSource.addTransferListener( + new TransferListener() { + @Override + public void onTransferInitializing( + DataSource source, DataSpec dataSpec, boolean isNetwork) { + throw new IllegalStateException(); + } + + @Override + public void onTransferStart(DataSource source, DataSpec dataSpec, boolean isNetwork) {} + + @Override + public void onBytesTransferred( + DataSource source, DataSpec dataSpec, boolean isNetwork, int bytesTransferred) {} + + @Override + public void onTransferEnd(DataSource source, DataSpec dataSpec, boolean isNetwork) {} + }); + CacheWriter cacheWriter = + new CacheWriter( + new CacheDataSource(cache, dataSource), + new DataSpec(Uri.parse("test_data")), + /* temporaryBuffer= */ null, + new CachingCounters()); + + assertThrows(IllegalStateException.class, cacheWriter::cache); + + assertThat(dataSource.isOpened()).isFalse(); + } + + @Test + public void cache_nonIoExceptionDuringRead_closesDataSource() { + FakeDataSet fakeDataSet = + new FakeDataSet() + .newData("test_data") + .appendReadAction( + () -> { + throw new IllegalStateException(); + }) + .appendReadData(1) + .endData(); + FakeDataSource dataSource = new FakeDataSource(fakeDataSet); + CacheWriter cacheWriter = + new CacheWriter( + new CacheDataSource(cache, dataSource), + new DataSpec(Uri.parse("test_data")), + /* temporaryBuffer= */ null, + new CachingCounters()); + + assertThrows(IllegalStateException.class, cacheWriter::cache); + + assertThat(dataSource.isOpened()).isFalse(); + } + private static final class CachingCounters implements CacheWriter.ProgressListener { private long contentLength = C.LENGTH_UNSET; diff --git a/libraries/exoplayer_dash/src/test/java/androidx/media3/exoplayer/dash/offline/DownloadManagerDashTest.java b/libraries/exoplayer_dash/src/test/java/androidx/media3/exoplayer/dash/offline/DownloadManagerDashTest.java index 746b577835..5267e9ea90 100644 --- a/libraries/exoplayer_dash/src/test/java/androidx/media3/exoplayer/dash/offline/DownloadManagerDashTest.java +++ b/libraries/exoplayer_dash/src/test/java/androidx/media3/exoplayer/dash/offline/DownloadManagerDashTest.java @@ -165,7 +165,7 @@ public class DownloadManagerDashTest { } @Test - public void handleDownloadRequest_withInferringRequest_success() throws Throwable { + public void handleDownloadRequest_withInterferingRequest_success() throws Throwable { fakeDataSet .newData("audio_segment_2") .appendReadAction(() -> handleDownloadRequest(fakeStreamKey2))