Be clearer about what types we're catching.

In particular, this helps make clearer that none of these particular `catch` blocks catch arbitrary checked exceptions—and thus that none of them catch `InterruptedException`. Also, I've removed some `catch (Throwable t)` blocks from `FakeTimeLimiter`. They could never be hit, since previous blocks had caught both `Error` and `Exception` (in the case of `Callable`) or `RuntimeException` (in the case of `Runnable`). RELNOTES=n/a PiperOrigin-RevId: 445538563
google · Apr 30, 2022 · f2bb171 · f2bb171
1 parent 6d986d1
commit f2bb171
Show file tree

Hide file tree

Showing 24 changed files with 47 additions and 57 deletions.
diff --git a/android/guava/src/com/google/common/util/concurrent/AbstractCatchingFuture.java b/android/guava/src/com/google/common/util/concurrent/AbstractCatchingFuture.java
@@ -107,7 +107,7 @@ public final void run() {
                     + e.getClass()
                     + " without a cause");
       }
-    } catch (Throwable e) { // this includes cancellation exception
+    } catch (RuntimeException | Error e) { // this includes cancellation exception
       throwable = e;
     }
 

diff --git a/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java b/android/guava/src/com/google/common/util/concurrent/AbstractFuture.java
@@ -15,7 +15,6 @@
 package com.google.common.util.concurrent;
 
 import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Throwables.throwIfUnchecked;
 import static com.google.common.util.concurrent.NullnessCasts.uncheckedNull;
 import static java.lang.Integer.toHexString;
 import static java.lang.System.identityHashCode;
@@ -159,7 +158,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
 
     try {
       helper = new UnsafeAtomicHelper();
-    } catch (Throwable unsafeFailure) {
+    } catch (RuntimeException | Error unsafeFailure) {
       thrownUnsafeFailure = unsafeFailure;
       // catch absolutely everything and fall through to our 'SafeAtomicHelper'
       // The access control checks that ARFU does means the caller class has to be AbstractFuture
@@ -172,7 +171,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
                 newUpdater(AbstractFuture.class, Waiter.class, "waiters"),
                 newUpdater(AbstractFuture.class, Listener.class, "listeners"),
                 newUpdater(AbstractFuture.class, Object.class, "value"));
-      } catch (Throwable atomicReferenceFieldUpdaterFailure) {
+      } catch (RuntimeException | Error atomicReferenceFieldUpdaterFailure) {
         // Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
         // getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
         // For these users fallback to a suboptimal implementation, based on synchronized. This will
@@ -859,14 +858,14 @@ protected boolean setFuture(ListenableFuture<? extends V> future) {
         // since all we are doing is unpacking a completed future which should be fast.
         try {
           future.addListener(valueToSet, DirectExecutor.INSTANCE);
-        } catch (Throwable t) {
+        } catch (RuntimeException | Error t) {
           // addListener has thrown an exception! SetFuture.run can't throw any exceptions so this
           // must have been caused by addListener itself. The most likely explanation is a
           // misconfigured mock. Try to switch to Failure.
           Failure failure;
           try {
             failure = new Failure(t);
-          } catch (Throwable oomMostLikely) {
+          } catch (RuntimeException | Error oomMostLikely) {
             failure = Failure.FALLBACK_INSTANCE;
           }
           // Note: The only way this CAS could fail is if cancel() has raced with us. That is ok.
@@ -961,7 +960,7 @@ private static Object getFutureValue(ListenableFuture<?> future) {
                 cancellation));
       }
       return new Cancellation(false, cancellation);
-    } catch (Throwable t) {
+    } catch (RuntimeException | Error t) {
       return new Failure(t);
     }
   }
@@ -1353,9 +1352,10 @@ public sun.misc.Unsafe run() throws Exception {
         WAITER_THREAD_OFFSET = unsafe.objectFieldOffset(Waiter.class.getDeclaredField("thread"));
         WAITER_NEXT_OFFSET = unsafe.objectFieldOffset(Waiter.class.getDeclaredField("next"));
         UNSAFE = unsafe;
-      } catch (Exception e) {
-        throwIfUnchecked(e);
+      } catch (NoSuchFieldException e) {
         throw new RuntimeException(e);
+      } catch (RuntimeException e) {
+        throw e;
       }
     }
 

diff --git a/android/guava/src/com/google/common/util/concurrent/AbstractScheduledService.java b/android/guava/src/com/google/common/util/concurrent/AbstractScheduledService.java
@@ -565,7 +565,7 @@ public Cancellable reschedule() {
         lock.lock();
         try {
           toReturn = initializeOrUpdateCancellationDelegate(schedule);
-        } catch (Throwable e) {
+        } catch (RuntimeException | Error e) {
           // If an exception is thrown by the subclass then we need to make sure that the service
           // notices and transitions to the FAILED state. We do it by calling notifyFailed directly
           // because the service does not monitor the state of the future so if the exception is not

diff --git a/android/guava/src/com/google/common/util/concurrent/AggregateFuture.java b/android/guava/src/com/google/common/util/concurrent/AggregateFuture.java
@@ -268,7 +268,7 @@ private void collectValueFromNonCancelledFuture(int index, Future<? extends Inpu
       collectOneValue(index, getDone(future));
     } catch (ExecutionException e) {
       handleException(e.getCause());
-    } catch (Throwable t) {
+    } catch (RuntimeException | Error t) {
       handleException(t);
     }
   }

diff --git a/android/guava/src/com/google/common/util/concurrent/AggregateFutureState.java b/android/guava/src/com/google/common/util/concurrent/AggregateFutureState.java
@@ -61,7 +61,7 @@ abstract class AggregateFutureState<OutputT extends @Nullable Object>
           new SafeAtomicHelper(
               newUpdater(AggregateFutureState.class, Set.class, "seenExceptions"),
               newUpdater(AggregateFutureState.class, "remaining"));
-    } catch (Throwable reflectionFailure) {
+    } catch (RuntimeException | Error reflectionFailure) {
       // Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
       // getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
       // For these users fallback to a suboptimal implementation, based on synchronized. This will

diff --git a/android/guava/src/com/google/common/util/concurrent/FakeTimeLimiter.java b/android/guava/src/com/google/common/util/concurrent/FakeTimeLimiter.java
@@ -62,10 +62,6 @@ public <T> T newProxy(
       throw new ExecutionException(e);
     } catch (Error e) {
       throw new ExecutionError(e);
-    } catch (Throwable e) {
-      // It's a non-Error, non-Exception Throwable. Such classes are usually intended to extend
-      // Exception, so we'll treat it like an Exception.
-      throw new ExecutionException(e);
     }
   }
 
@@ -86,10 +82,6 @@ public void runWithTimeout(Runnable runnable, long timeoutDuration, TimeUnit tim
       throw new UncheckedExecutionException(e);
     } catch (Error e) {
       throw new ExecutionError(e);
-    } catch (Throwable e) {
-      // It's a non-Error, non-Exception Throwable. Such classes are usually intended to extend
-      // Exception, so we'll treat it like a RuntimeException.
-      throw new UncheckedExecutionException(e);
     }
   }
 

diff --git a/android/guava/src/com/google/common/util/concurrent/Futures.java b/android/guava/src/com/google/common/util/concurrent/Futures.java
@@ -516,7 +516,7 @@ public O get(long timeout, TimeUnit unit)
       private O applyTransformation(I input) throws ExecutionException {
         try {
           return function.apply(input);
-        } catch (Throwable t) {
+        } catch (RuntimeException | Error t) {
           throw new ExecutionException(t);
         }
       }

diff --git a/android/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java b/android/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java
@@ -182,7 +182,7 @@ private static boolean hasConstructorUsableByGetChecked(
     try {
       Exception unused = newWithCause(exceptionClass, new Exception());
       return true;
-    } catch (Exception e) {
+    } catch (RuntimeException | Error e) {
       return false;
     }
   }

diff --git a/android/guava/src/com/google/common/util/concurrent/JdkFutureAdapters.java b/android/guava/src/com/google/common/util/concurrent/JdkFutureAdapters.java
@@ -19,6 +19,7 @@
 
 import com.google.common.annotations.Beta;
 import com.google.common.annotations.GwtIncompatible;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
@@ -158,9 +159,10 @@ public void addListener(Runnable listener, Executor exec) {
                  * to return a proper ListenableFuture instead of using listenInPoolThread.
                  */
                 getUninterruptibly(delegate);
-              } catch (Throwable e) {
-                // ExecutionException / CancellationException / RuntimeException / Error
+              } catch (ExecutionException | RuntimeException | Error e) {
+                // (including CancellationException)
                 // The task is presumably done, run the listeners.
+                // TODO(cpovirk): Do *something* in case of Error (and maybe RuntimeException)?
               }
               executionList.execute();
             });

diff --git a/android/guava/src/com/google/common/util/concurrent/Monitor.java b/android/guava/src/com/google/common/util/concurrent/Monitor.java
@@ -1011,7 +1011,7 @@ private void signalNextWaiter() {
   private boolean isSatisfied(Guard guard) {
     try {
       return guard.isSatisfied();
-    } catch (Throwable throwable) {
+    } catch (RuntimeException | Error throwable) {
       signalAllWaiters();
       throw throwable;
     }

diff --git a/android/guava/src/com/google/common/util/concurrent/MoreExecutors.java b/android/guava/src/com/google/common/util/concurrent/MoreExecutors.java
@@ -667,9 +667,9 @@ public NeverSuccessfulListenableFutureTask(Runnable delegate) {
       public void run() {
         try {
           delegate.run();
-        } catch (Throwable t) {
+        } catch (RuntimeException | Error t) {
           setException(t);
-          throw Throwables.propagate(t);
+          throw t;
         }
       }
 

diff --git a/android/guava/src/com/google/common/util/concurrent/UncaughtExceptionHandlers.java b/android/guava/src/com/google/common/util/concurrent/UncaughtExceptionHandlers.java
@@ -68,7 +68,7 @@ public void uncaughtException(Thread t, Throwable e) {
       try {
         logger.log(
             SEVERE, String.format(Locale.ROOT, "Caught an exception in %s.  Shutting down.", t), e);
-      } catch (Throwable errorInLogging) {
+      } catch (RuntimeException | Error errorInLogging) {
         // If logging fails, e.g. due to missing memory, at least try to log the
         // message and the cause for the failed logging.
         System.err.println(e.getMessage());

diff --git a/guava/src/com/google/common/util/concurrent/AbstractCatchingFuture.java b/guava/src/com/google/common/util/concurrent/AbstractCatchingFuture.java
@@ -107,7 +107,7 @@ public final void run() {
                     + e.getClass()
                     + " without a cause");
       }
-    } catch (Throwable e) { // this includes cancellation exception
+    } catch (RuntimeException | Error e) { // this includes cancellation exception
       throwable = e;
     }
 

diff --git a/guava/src/com/google/common/util/concurrent/AbstractFuture.java b/guava/src/com/google/common/util/concurrent/AbstractFuture.java
@@ -15,7 +15,6 @@
 package com.google.common.util.concurrent;
 
 import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Throwables.throwIfUnchecked;
 import static com.google.common.util.concurrent.NullnessCasts.uncheckedNull;
 import static java.lang.Integer.toHexString;
 import static java.lang.System.identityHashCode;
@@ -159,7 +158,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
 
     try {
       helper = new UnsafeAtomicHelper();
-    } catch (Throwable unsafeFailure) {
+    } catch (RuntimeException | Error unsafeFailure) {
       thrownUnsafeFailure = unsafeFailure;
       // catch absolutely everything and fall through to our 'SafeAtomicHelper'
       // The access control checks that ARFU does means the caller class has to be AbstractFuture
@@ -172,7 +171,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) {
                 newUpdater(AbstractFuture.class, Waiter.class, "waiters"),
                 newUpdater(AbstractFuture.class, Listener.class, "listeners"),
                 newUpdater(AbstractFuture.class, Object.class, "value"));
-      } catch (Throwable atomicReferenceFieldUpdaterFailure) {
+      } catch (RuntimeException | Error atomicReferenceFieldUpdaterFailure) {
         // Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
         // getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
         // For these users fallback to a suboptimal implementation, based on synchronized. This will
@@ -859,14 +858,14 @@ protected boolean setFuture(ListenableFuture<? extends V> future) {
         // since all we are doing is unpacking a completed future which should be fast.
         try {
           future.addListener(valueToSet, DirectExecutor.INSTANCE);
-        } catch (Throwable t) {
+        } catch (RuntimeException | Error t) {
           // addListener has thrown an exception! SetFuture.run can't throw any exceptions so this
           // must have been caused by addListener itself. The most likely explanation is a
           // misconfigured mock. Try to switch to Failure.
           Failure failure;
           try {
             failure = new Failure(t);
-          } catch (Throwable oomMostLikely) {
+          } catch (RuntimeException | Error oomMostLikely) {
             failure = Failure.FALLBACK_INSTANCE;
           }
           // Note: The only way this CAS could fail is if cancel() has raced with us. That is ok.
@@ -961,7 +960,7 @@ private static Object getFutureValue(ListenableFuture<?> future) {
                 cancellation));
       }
       return new Cancellation(false, cancellation);
-    } catch (Throwable t) {
+    } catch (RuntimeException | Error t) {
       return new Failure(t);
     }
   }
@@ -1353,9 +1352,10 @@ public sun.misc.Unsafe run() throws Exception {
         WAITER_THREAD_OFFSET = unsafe.objectFieldOffset(Waiter.class.getDeclaredField("thread"));
         WAITER_NEXT_OFFSET = unsafe.objectFieldOffset(Waiter.class.getDeclaredField("next"));
         UNSAFE = unsafe;
-      } catch (Exception e) {
-        throwIfUnchecked(e);
+      } catch (NoSuchFieldException e) {
         throw new RuntimeException(e);
+      } catch (RuntimeException e) {
+        throw e;
       }
     }
 

diff --git a/guava/src/com/google/common/util/concurrent/AbstractScheduledService.java b/guava/src/com/google/common/util/concurrent/AbstractScheduledService.java
@@ -607,7 +607,7 @@ public Cancellable reschedule() {
         lock.lock();
         try {
           toReturn = initializeOrUpdateCancellationDelegate(schedule);
-        } catch (Throwable e) {
+        } catch (RuntimeException | Error e) {
           // If an exception is thrown by the subclass then we need to make sure that the service
           // notices and transitions to the FAILED state. We do it by calling notifyFailed directly
           // because the service does not monitor the state of the future so if the exception is not

diff --git a/guava/src/com/google/common/util/concurrent/AggregateFuture.java b/guava/src/com/google/common/util/concurrent/AggregateFuture.java
@@ -268,7 +268,7 @@ private void collectValueFromNonCancelledFuture(int index, Future<? extends Inpu
       collectOneValue(index, getDone(future));
     } catch (ExecutionException e) {
       handleException(e.getCause());
-    } catch (Throwable t) {
+    } catch (RuntimeException | Error t) {
       handleException(t);
     }
   }

diff --git a/guava/src/com/google/common/util/concurrent/AggregateFutureState.java b/guava/src/com/google/common/util/concurrent/AggregateFutureState.java
@@ -61,7 +61,7 @@ abstract class AggregateFutureState<OutputT extends @Nullable Object>
           new SafeAtomicHelper(
               newUpdater(AggregateFutureState.class, Set.class, "seenExceptions"),
               newUpdater(AggregateFutureState.class, "remaining"));
-    } catch (Throwable reflectionFailure) {
+    } catch (RuntimeException | Error reflectionFailure) {
       // Some Android 5.0.x Samsung devices have bugs in JDK reflection APIs that cause
       // getDeclaredField to throw a NoSuchFieldException when the field is definitely there.
       // For these users fallback to a suboptimal implementation, based on synchronized. This will

diff --git a/guava/src/com/google/common/util/concurrent/FakeTimeLimiter.java b/guava/src/com/google/common/util/concurrent/FakeTimeLimiter.java
@@ -62,10 +62,6 @@ public <T> T newProxy(
       throw new ExecutionException(e);
     } catch (Error e) {
       throw new ExecutionError(e);
-    } catch (Throwable e) {
-      // It's a non-Error, non-Exception Throwable. Such classes are usually intended to extend
-      // Exception, so we'll treat it like an Exception.
-      throw new ExecutionException(e);
     }
   }
 
@@ -86,10 +82,6 @@ public void runWithTimeout(Runnable runnable, long timeoutDuration, TimeUnit tim
       throw new UncheckedExecutionException(e);
     } catch (Error e) {
       throw new ExecutionError(e);
-    } catch (Throwable e) {
-      // It's a non-Error, non-Exception Throwable. Such classes are usually intended to extend
-      // Exception, so we'll treat it like a RuntimeException.
-      throw new UncheckedExecutionException(e);
     }
   }
 

diff --git a/guava/src/com/google/common/util/concurrent/Futures.java b/guava/src/com/google/common/util/concurrent/Futures.java
@@ -549,7 +549,7 @@ public O get(long timeout, TimeUnit unit)
       private O applyTransformation(I input) throws ExecutionException {
         try {
           return function.apply(input);
-        } catch (Throwable t) {
+        } catch (RuntimeException | Error t) {
           throw new ExecutionException(t);
         }
       }

diff --git a/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java b/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java
@@ -193,7 +193,9 @@ static GetCheckedTypeValidator getBestValidator() {
         Class<? extends Enum> theClass =
             Class.forName(CLASS_VALUE_VALIDATOR_NAME).asSubclass(Enum.class);
         return (GetCheckedTypeValidator) theClass.getEnumConstants()[0];
-      } catch (Throwable t) { // ensure we really catch *everything*
+      } catch (ClassNotFoundException
+          | RuntimeException
+          | Error t) { // ensure we really catch *everything*
         return weakSetValidator();
       }
     }
@@ -221,7 +223,7 @@ private static boolean hasConstructorUsableByGetChecked(
     try {
       Exception unused = newWithCause(exceptionClass, new Exception());
       return true;
-    } catch (Exception e) {
+    } catch (RuntimeException | Error e) {
       return false;
     }
   }

diff --git a/guava/src/com/google/common/util/concurrent/JdkFutureAdapters.java b/guava/src/com/google/common/util/concurrent/JdkFutureAdapters.java
@@ -19,6 +19,7 @@
 
 import com.google.common.annotations.Beta;
 import com.google.common.annotations.GwtIncompatible;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
@@ -158,9 +159,10 @@ public void addListener(Runnable listener, Executor exec) {
                  * to return a proper ListenableFuture instead of using listenInPoolThread.
                  */
                 getUninterruptibly(delegate);
-              } catch (Throwable e) {
-                // ExecutionException / CancellationException / RuntimeException / Error
+              } catch (ExecutionException | RuntimeException | Error e) {
+                // (including CancellationException)
                 // The task is presumably done, run the listeners.
+                // TODO(cpovirk): Do *something* in case of Error (and maybe RuntimeException)?
               }
               executionList.execute();
             });

diff --git a/guava/src/com/google/common/util/concurrent/Monitor.java b/guava/src/com/google/common/util/concurrent/Monitor.java
@@ -1121,7 +1121,7 @@ private void signalNextWaiter() {
   private boolean isSatisfied(Guard guard) {
     try {
       return guard.isSatisfied();
-    } catch (Throwable throwable) {
+    } catch (RuntimeException | Error throwable) {
       signalAllWaiters();
       throw throwable;
     }

diff --git a/guava/src/com/google/common/util/concurrent/MoreExecutors.java b/guava/src/com/google/common/util/concurrent/MoreExecutors.java
@@ -728,9 +728,9 @@ public NeverSuccessfulListenableFutureTask(Runnable delegate) {
       public void run() {
         try {
           delegate.run();
-        } catch (Throwable t) {
+        } catch (RuntimeException | Error t) {
           setException(t);
-          throw Throwables.propagate(t);
+          throw t;
         }
       }
 

diff --git a/guava/src/com/google/common/util/concurrent/UncaughtExceptionHandlers.java b/guava/src/com/google/common/util/concurrent/UncaughtExceptionHandlers.java
@@ -68,7 +68,7 @@ public void uncaughtException(Thread t, Throwable e) {
       try {
         logger.log(
             SEVERE, String.format(Locale.ROOT, "Caught an exception in %s.  Shutting down.", t), e);
-      } catch (Throwable errorInLogging) {
+      } catch (RuntimeException | Error errorInLogging) {
         // If logging fails, e.g. due to missing memory, at least try to log the
         // message and the cause for the failed logging.
         System.err.println(e.getMessage());