From 073e50a893b6bace45d7fec4d180279e69933a7e Mon Sep 17 00:00:00 2001 From: tonihei Date: Fri, 21 Feb 2025 02:09:21 -0800 Subject: [PATCH] Ensure DataSource is closed in CacheWriter at non-IOException We currently only catch IOExceptions to close the opened data source. This means any other type of exception leaves the data source open, which can caused SimpleCache to be blocked forever when another data source tries to access the same cache area (which is still locked by the open data source). This problem was the cause for flakiness in DownloadManagerDashTest. #cherrypick Issue: google/ExoPlayer#9760 PiperOrigin-RevId: 729441817 (cherry picked from commit 72c85aa48393e47642688c8147bdf18d6e3d4bbf) --- RELEASENOTES.md | 4 + .../media3/datasource/cache/CacheWriter.java | 6 +- .../datasource/cache/CacheWriterTest.java | 115 ++++++++++++++++++ .../dash/offline/DownloadManagerDashTest.java | 2 +- 4 files changed, 123 insertions(+), 4 deletions(-) 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))