Fix interrupt handling in the Condition overloads of Uninterruptibles.awaitUninterruptibly.

(extremely belated follow-up to #1212 / #2870 / #3010)

(Plus, I addressed one http://errorprone.info/bugpattern/LockNotBeforeTry warning in the test. I don't want to think about the http://errorprone.info/bugpattern/WaitNotInLoop warnings right now, but I did leave a comment about them.)

This bug felt worth explaining in the comments in the code itself, so I'll leave the explanation to them.

RELNOTES=n/a
PiperOrigin-RevId: 862924189

Author: cpovirk

android/guava-tests/test/com/google/common/util/concurrent/UninterruptiblesTest.java

@@ -117,13 +117,19 @@ public void testNull() throws Exception {
   // CountDownLatch.await() tests
 
   // Condition.await() tests
+
+  /*
+   * Our tests for awaitUninterruptibly are written under the assumption that no spurious wakeups
+   * occur except for those produced by awaitUninterruptibly itself in response to interrupts.
+   */
+
   public void testConditionAwaitTimeoutExceeded() {
     Stopwatch stopwatch = Stopwatch.createStarted();
     Condition condition = TestCondition.create();
 
-    boolean signaledBeforeTimeout = awaitUninterruptibly(condition, 500, MILLISECONDS);
+    boolean returnedBeforeTimeout = awaitUninterruptibly(condition, 500, MILLISECONDS);
 
-    assertFalse(signaledBeforeTimeout);
+    assertFalse(returnedBeforeTimeout);
     assertAtLeastTimePassed(stopwatch, 500);
     assertNotInterrupted();
   }
@@ -132,9 +138,9 @@ public void testConditionAwaitTimeoutNotExceeded() {
     Stopwatch stopwatch = Stopwatch.createStarted();
     Condition condition = TestCondition.createAndSignalAfter(500, MILLISECONDS);
 
-    boolean signaledBeforeTimeout = awaitUninterruptibly(condition, 1500, MILLISECONDS);
+    boolean returnedBeforeTimeout = awaitUninterruptibly(condition, 1500, MILLISECONDS);
 
-    assertTrue(signaledBeforeTimeout);
+    assertTrue(returnedBeforeTimeout);
     assertTimeNotPassed(stopwatch, LONG_DELAY_MS);
     assertNotInterrupted();
   }
@@ -144,10 +150,10 @@ public void testConditionAwaitInterruptedTimeoutExceeded() {
     Condition condition = TestCondition.create();
     requestInterruptIn(500);
 
-    boolean signaledBeforeTimeout = awaitUninterruptibly(condition, 1000, MILLISECONDS);
+    boolean returnedBeforeTimeout = awaitUninterruptibly(condition, 1000, MILLISECONDS);
 
-    assertFalse(signaledBeforeTimeout);
-    assertAtLeastTimePassed(stopwatch, 1000);
+    assertTrue(returnedBeforeTimeout);
+    assertAtLeastTimePassed(stopwatch, 500);
     assertInterrupted();
   }
 
@@ -156,9 +162,21 @@ public void testConditionAwaitInterruptedTimeoutNotExceeded() {
     Condition condition = TestCondition.createAndSignalAfter(1000, MILLISECONDS);
     requestInterruptIn(500);
 
-    boolean signaledBeforeTimeout = awaitUninterruptibly(condition, 1500, MILLISECONDS);
+    boolean returnedBeforeTimeout = awaitUninterruptibly(condition, 1500, MILLISECONDS);
 
-    assertTrue(signaledBeforeTimeout);
+    assertTrue(returnedBeforeTimeout);
+    assertTimeNotPassed(stopwatch, LONG_DELAY_MS);
+    assertInterrupted();
+  }
+
+  public void testConditionAwaitMultiInterrupt() {
+    Stopwatch stopwatch = Stopwatch.createStarted();
+    Condition condition = TestCondition.createAndSignalAfter(1000, MILLISECONDS);
+    repeatedlyInterruptTestThread(tearDownStack);
+
+    boolean returnedBeforeTimeout = awaitUninterruptibly(condition, Duration.ofHours(1));
+
+    assertTrue(returnedBeforeTimeout);
     assertTimeNotPassed(stopwatch, LONG_DELAY_MS);
     assertInterrupted();
   }
@@ -184,9 +202,9 @@ public void testTryLockTimeoutNotExceeded() {
     Lock lock = new ReentrantLock();
     acquireFor(lock, 500, MILLISECONDS);
 
-    boolean signaledBeforeTimeout = tryLockUninterruptibly(lock, 1500, MILLISECONDS);
+    boolean acquired = tryLockUninterruptibly(lock, 1500, MILLISECONDS);
 
-    assertTrue(signaledBeforeTimeout);
+    assertTrue(acquired);
     assertTimeNotPassed(stopwatch, LONG_DELAY_MS);
     assertNotInterrupted();
   }
@@ -197,9 +215,9 @@ public void testTryLockInterruptedTimeoutExceeded() {
     Thread lockThread = acquireFor(lock, 5, SECONDS);
     requestInterruptIn(500);
 
-    boolean signaledBeforeTimeout = tryLockUninterruptibly(lock, 1000, MILLISECONDS);
+    boolean acquired = tryLockUninterruptibly(lock, 1000, MILLISECONDS);
 
-    assertFalse(signaledBeforeTimeout);
+    assertFalse(acquired);
     assertAtLeastTimePassed(stopwatch, 1000);
     assertInterrupted();
 
@@ -213,9 +231,9 @@ public void testTryLockInterruptedTimeoutNotExceeded() {
     acquireFor(lock, 1000, MILLISECONDS);
     requestInterruptIn(500);
 
-    boolean signaledBeforeTimeout = tryLockUninterruptibly(lock, 1500, MILLISECONDS);
+    boolean acquired = tryLockUninterruptibly(lock, 1500, MILLISECONDS);
 
-    assertTrue(signaledBeforeTimeout);
+    assertTrue(acquired);
     assertTimeNotPassed(stopwatch, LONG_DELAY_MS);
     assertInterrupted();
   }
@@ -876,8 +894,9 @@ private static Thread acquireFor(Lock lock, long duration, TimeUnit unit) {
           @Override
           public void run() {
             lock.lock();
-            latch.countDown();
             try {
+
+              latch.countDown();
               Thread.sleep(unit.toMillis(duration));
             } catch (InterruptedException e) {
               // simply finish execution

android/guava/src/com/google/common/util/concurrent/Uninterruptibles.java

@@ -47,10 +47,6 @@
  */
 @GwtCompatible
 public final class Uninterruptibles {
-
-  // Implementation Note: As of 3-7-11, the logic for each blocking/timeout
-  // methods is identical, save for method being invoked.
-
   /** Invokes {@code latch.}{@link CountDownLatch#await() await()} uninterruptibly. */
   @J2ktIncompatible
   @GwtIncompatible // concurrency
@@ -115,9 +111,18 @@ public static boolean awaitUninterruptibly(CountDownLatch latch, long timeout, T
   }
 
   /**
-   * Invokes {@code condition.}{@link Condition#await(long, TimeUnit) await(timeout, unit)}
-   * uninterruptibly.
+   * Invokes {@code condition.}{@link Condition#await(long, TimeUnit) await(timeout, unit)} in a way
+   * that more conveniently supports uninterruptible waits.
    *
+   * <p>If the underlying {@code await} call is interrupted, then {@code awaitUninterruptibly}
+   * converts that into a <a
+   * href="https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/util/concurrent/locks/Condition.html#implementation-considerations-heading">spurious
+   * wakeup</a>. This means that resulting wait is not "uninterruptible" in the normal sense of
+   * {@link Uninterruptibles}. Still, this method allows callers to write <a
+   * href="https://errorprone.info/bugpattern/WaitNotInLoop">the standard, required loop for waiting on a {@code
+   * Condition}</a> but without the need to handle interruption.
+   *
+   * @return {@code false} if the waiting time detectably elapsed before return from the method
    * @since 33.4.0 (but since 28.0 in the JRE flavor)
    */
   @J2ktIncompatible
@@ -128,32 +133,58 @@ public static boolean awaitUninterruptibly(Condition condition, Duration timeout
   }
 
   /**
-   * Invokes {@code condition.}{@link Condition#await(long, TimeUnit) await(timeout, unit)}
-   * uninterruptibly.
+   * Invokes {@code condition.}{@link Condition#await(long, TimeUnit) await(timeout, unit)} in a way
+   * that more conveniently supports uninterruptible waits.
    *
+   * <p>If the underlying {@code await} call is interrupted, then {@code awaitUninterruptibly}
+   * converts that into a <a
+   * href="https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/util/concurrent/locks/Condition.html#implementation-considerations-heading">spurious
+   * wakeup</a>. This means that resulting wait is not "uninterruptible" in the normal sense of
+   * {@link Uninterruptibles}. Still, this method allows callers to write <a
+   * href="https://errorprone.info/bugpattern/WaitNotInLoop">the standard, required loop for waiting on a {@code
+   * Condition}</a> but without the need to handle interruption.
+   *
+   * @return {@code false} if the waiting time detectably elapsed before return from the method
    * @since 23.6
    */
   @J2ktIncompatible
   @GwtIncompatible // concurrency
   @SuppressWarnings("GoodTime") // should accept a java.time.Duration
   public static boolean awaitUninterruptibly(Condition condition, long timeout, TimeUnit unit) {
-    boolean interrupted = false;
-    try {
-      long remainingNanos = unit.toNanos(timeout);
-      long end = System.nanoTime() + remainingNanos;
+    /*
+     * An uninterruptible wait on a Condition requires different logic than an uninterruptible wait
+     * on most other types: In cases in which we "should" receive both an interrupt and a
+     * notification nearly simultaneously, we sometimes receive only an interrupt. Thus, when we're
+     * interrupted, we can't just poll whether it's time to end the wait because our "end the wait"
+     * notification has been lost. (This is in contrast to how we can poll with, say, a
+     * CountDownLatch.) In order to avoid hiding the requested notification from the caller, we need
+     * to return. Fortunately, a wait on a Condition is allowed to return early on account of a
+     * "spurious wakeup," so we're allowed to convert interruptions into such wakeups.
+     */
+
+    /*
+     * Since we can't loop inside awaitUninterruptibly(Condition, ...), the user is responsible for
+     * calling us again in case of interrupt. Then, if we were to call await(...) immediately, as we
+     * do in the other Uninterruptibles methods, it would throw immediately. Then we'd restore the
+     * interrupt and return again, and the user would call us again, creating a busy wait.
+     *
+     * Thus, we need to clear the interrupt eagerly in case it's an interrupt from a previous call
+     * to awaitUninterruptibly in the user code's Condition loop.
+     */
+    boolean wasAlreadyInterrupted = Thread.interrupted();
+    long remainingNanos = unit.toNanos(timeout);
+    long end = System.nanoTime() + remainingNanos;
 
-      while (true) {
-        try {
-          return condition.await(remainingNanos, NANOSECONDS);
-        } catch (InterruptedException e) {
-          interrupted = true;
-          remainingNanos = end - System.nanoTime();
-        }
-      }
-    } finally {
-      if (interrupted) {
+    try {
+      boolean result = condition.await(remainingNanos, NANOSECONDS);
+      if (wasAlreadyInterrupted) {
         Thread.currentThread().interrupt();
       }
+      return result;
+    } catch (InterruptedException e) {
+      Thread.currentThread().interrupt();
+      // better than `end > System.nanoTime()` because `System.nanoTime()` could wrap around
+      return end - System.nanoTime() > 0;
     }
   }
 

guava-tests/test/com/google/common/util/concurrent/UninterruptiblesTest.java

@@ -117,13 +117,19 @@ public void testNull() throws Exception {
   // CountDownLatch.await() tests
 
   // Condition.await() tests
+
+  /*
+   * Our tests for awaitUninterruptibly are written under the assumption that no spurious wakeups
+   * occur except for those produced by awaitUninterruptibly itself in response to interrupts.
+   */
+
   public void testConditionAwaitTimeoutExceeded() {
     Stopwatch stopwatch = Stopwatch.createStarted();
     Condition condition = TestCondition.create();
 
-    boolean signaledBeforeTimeout = awaitUninterruptibly(condition, 500, MILLISECONDS);
+    boolean returnedBeforeTimeout = awaitUninterruptibly(condition, 500, MILLISECONDS);
 
-    assertFalse(signaledBeforeTimeout);
+    assertFalse(returnedBeforeTimeout);
     assertAtLeastTimePassed(stopwatch, 500);
     assertNotInterrupted();
   }
@@ -132,9 +138,9 @@ public void testConditionAwaitTimeoutNotExceeded() {
     Stopwatch stopwatch = Stopwatch.createStarted();
     Condition condition = TestCondition.createAndSignalAfter(500, MILLISECONDS);
 
-    boolean signaledBeforeTimeout = awaitUninterruptibly(condition, 1500, MILLISECONDS);
+    boolean returnedBeforeTimeout = awaitUninterruptibly(condition, 1500, MILLISECONDS);
 
-    assertTrue(signaledBeforeTimeout);
+    assertTrue(returnedBeforeTimeout);
     assertTimeNotPassed(stopwatch, LONG_DELAY_MS);
     assertNotInterrupted();
   }
@@ -144,10 +150,10 @@ public void testConditionAwaitInterruptedTimeoutExceeded() {
     Condition condition = TestCondition.create();
     requestInterruptIn(500);
 
-    boolean signaledBeforeTimeout = awaitUninterruptibly(condition, 1000, MILLISECONDS);
+    boolean returnedBeforeTimeout = awaitUninterruptibly(condition, 1000, MILLISECONDS);
 
-    assertFalse(signaledBeforeTimeout);
-    assertAtLeastTimePassed(stopwatch, 1000);
+    assertTrue(returnedBeforeTimeout);
+    assertAtLeastTimePassed(stopwatch, 500);
     assertInterrupted();
   }
 
@@ -156,9 +162,21 @@ public void testConditionAwaitInterruptedTimeoutNotExceeded() {
     Condition condition = TestCondition.createAndSignalAfter(1000, MILLISECONDS);
     requestInterruptIn(500);
 
-    boolean signaledBeforeTimeout = awaitUninterruptibly(condition, 1500, MILLISECONDS);
+    boolean returnedBeforeTimeout = awaitUninterruptibly(condition, 1500, MILLISECONDS);
 
-    assertTrue(signaledBeforeTimeout);
+    assertTrue(returnedBeforeTimeout);
+    assertTimeNotPassed(stopwatch, LONG_DELAY_MS);
+    assertInterrupted();
+  }
+
+  public void testConditionAwaitMultiInterrupt() {
+    Stopwatch stopwatch = Stopwatch.createStarted();
+    Condition condition = TestCondition.createAndSignalAfter(1000, MILLISECONDS);
+    repeatedlyInterruptTestThread(tearDownStack);
+
+    boolean returnedBeforeTimeout = awaitUninterruptibly(condition, Duration.ofHours(1));
+
+    assertTrue(returnedBeforeTimeout);
     assertTimeNotPassed(stopwatch, LONG_DELAY_MS);
     assertInterrupted();
   }
@@ -184,9 +202,9 @@ public void testTryLockTimeoutNotExceeded() {
     Lock lock = new ReentrantLock();
     acquireFor(lock, 500, MILLISECONDS);
 
-    boolean signaledBeforeTimeout = tryLockUninterruptibly(lock, 1500, MILLISECONDS);
+    boolean acquired = tryLockUninterruptibly(lock, 1500, MILLISECONDS);
 
-    assertTrue(signaledBeforeTimeout);
+    assertTrue(acquired);
     assertTimeNotPassed(stopwatch, LONG_DELAY_MS);
     assertNotInterrupted();
   }
@@ -197,9 +215,9 @@ public void testTryLockInterruptedTimeoutExceeded() {
     Thread lockThread = acquireFor(lock, 5, SECONDS);
     requestInterruptIn(500);
 
-    boolean signaledBeforeTimeout = tryLockUninterruptibly(lock, 1000, MILLISECONDS);
+    boolean acquired = tryLockUninterruptibly(lock, 1000, MILLISECONDS);
 
-    assertFalse(signaledBeforeTimeout);
+    assertFalse(acquired);
     assertAtLeastTimePassed(stopwatch, 1000);
     assertInterrupted();
 
@@ -213,9 +231,9 @@ public void testTryLockInterruptedTimeoutNotExceeded() {
     acquireFor(lock, 1000, MILLISECONDS);
     requestInterruptIn(500);
 
-    boolean signaledBeforeTimeout = tryLockUninterruptibly(lock, 1500, MILLISECONDS);
+    boolean acquired = tryLockUninterruptibly(lock, 1500, MILLISECONDS);
 
-    assertTrue(signaledBeforeTimeout);
+    assertTrue(acquired);
     assertTimeNotPassed(stopwatch, LONG_DELAY_MS);
     assertInterrupted();
   }
@@ -876,8 +894,9 @@ private static Thread acquireFor(Lock lock, long duration, TimeUnit unit) {
           @Override
           public void run() {
             lock.lock();
-            latch.countDown();
             try {
+
+              latch.countDown();
               Thread.sleep(unit.toMillis(duration));
             } catch (InterruptedException e) {
               // simply finish execution