From 03ea39b17521263a99b163d0c004947cece051f7 Mon Sep 17 00:00:00 2001 From: olly Date: Tue, 26 May 2020 10:04:03 +0100 Subject: [PATCH] ConditionVariable: Add uninterruptible block Sometimes it's useful to be able to block until something on some other thread "really has finished". This will be needed for moving caching operations onto the executor in Downloader implementations, since we need to guarantee that Downloader.download doesn't return until it's no longer modifying the underlying cache. One solution to this is of course just to not interrupt the thread that's blocking on the condition variable, but there are cases where you do need to do this in case the thread is at some other point in its execution. This is true for Downloader implementations, where the Download.download thread will also be blocking on PriorityTaskManager.proceed. Arranging to conditionally interrupt the thread based on where it's got to is probably possible, but seems complicated and error prone. Issue: #5978 PiperOrigin-RevId: 313152413 --- .../exoplayer2/util/ConditionVariable.java | 20 +++++++++ .../util/ConditionVariableTest.java | 41 ++++++++++++++++--- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/library/core/src/main/java/com/google/android/exoplayer2/util/ConditionVariable.java b/library/core/src/main/java/com/google/android/exoplayer2/util/ConditionVariable.java index b7f0d04e239..e5665c76ffd 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/util/ConditionVariable.java +++ b/library/core/src/main/java/com/google/android/exoplayer2/util/ConditionVariable.java @@ -111,6 +111,26 @@ public synchronized boolean block(long timeoutMs) throws InterruptedException { return isOpen; } + /** + * Blocks until the condition is open. Unlike {@link #block}, this method will continue to block + * if the calling thread is interrupted. If the calling thread was interrupted then its {@link + * Thread#isInterrupted() interrupted status} will be set when the method returns. + */ + public synchronized void blockUninterruptible() { + boolean wasInterrupted = false; + while (!isOpen) { + try { + wait(); + } catch (InterruptedException e) { + wasInterrupted = true; + } + } + if (wasInterrupted) { + // Restore the interrupted status. + Thread.currentThread().interrupt(); + } + } + /** Returns whether the condition is opened. */ public synchronized boolean isOpen() { return isOpen; diff --git a/library/core/src/test/java/com/google/android/exoplayer2/util/ConditionVariableTest.java b/library/core/src/test/java/com/google/android/exoplayer2/util/ConditionVariableTest.java index 8f2fb2ed14b..e7e0d8911a2 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/util/ConditionVariableTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/util/ConditionVariableTest.java @@ -49,7 +49,7 @@ public void blockWithTimeout_blocksForAtLeastTimeout() throws InterruptedExcepti } @Test - public void blockWithoutTimeout_blocks() throws InterruptedException { + public void blockWithMaxTimeout_blocks_thenThrowsWhenInterrupted() throws InterruptedException { ConditionVariable conditionVariable = buildTestConditionVariable(); AtomicBoolean blockReturned = new AtomicBoolean(); @@ -58,7 +58,7 @@ public void blockWithoutTimeout_blocks() throws InterruptedException { new Thread( () -> { try { - conditionVariable.block(); + conditionVariable.block(/* timeoutMs= */ Long.MAX_VALUE); blockReturned.set(true); } catch (InterruptedException e) { blockWasInterrupted.set(true); @@ -76,7 +76,7 @@ public void blockWithoutTimeout_blocks() throws InterruptedException { } @Test - public void blockWithMaxTimeout_blocks() throws InterruptedException { + public void block_blocks_thenThrowsWhenInterrupted() throws InterruptedException { ConditionVariable conditionVariable = buildTestConditionVariable(); AtomicBoolean blockReturned = new AtomicBoolean(); @@ -85,7 +85,7 @@ public void blockWithMaxTimeout_blocks() throws InterruptedException { new Thread( () -> { try { - conditionVariable.block(/* timeoutMs= */ Long.MAX_VALUE); + conditionVariable.block(); blockReturned.set(true); } catch (InterruptedException e) { blockWasInterrupted.set(true); @@ -103,7 +103,7 @@ public void blockWithMaxTimeout_blocks() throws InterruptedException { } @Test - public void open_unblocksBlock() throws InterruptedException { + public void block_blocks_thenReturnsWhenOpened() throws InterruptedException { ConditionVariable conditionVariable = buildTestConditionVariable(); AtomicBoolean blockReturned = new AtomicBoolean(); @@ -129,6 +129,37 @@ public void open_unblocksBlock() throws InterruptedException { assertThat(conditionVariable.isOpen()).isTrue(); } + @Test + public void blockUnterruptible_blocksIfInterrupted_thenUnblocksWhenOpened() + throws InterruptedException { + ConditionVariable conditionVariable = buildTestConditionVariable(); + + AtomicBoolean blockReturned = new AtomicBoolean(); + AtomicBoolean interruptedStatusSet = new AtomicBoolean(); + Thread blockingThread = + new Thread( + () -> { + conditionVariable.blockUninterruptible(); + blockReturned.set(true); + interruptedStatusSet.set(Thread.currentThread().isInterrupted()); + }); + + blockingThread.start(); + Thread.sleep(500); + assertThat(blockReturned.get()).isFalse(); + + blockingThread.interrupt(); + Thread.sleep(500); + // blockUninterruptible should still be blocked. + assertThat(blockReturned.get()).isFalse(); + + conditionVariable.open(); + blockingThread.join(); + // blockUninterruptible should have set the thread's interrupted status on exit. + assertThat(interruptedStatusSet.get()).isTrue(); + assertThat(conditionVariable.isOpen()).isTrue(); + } + private static ConditionVariable buildTestConditionVariable() { return new ConditionVariable( new SystemClock() {