Make AggregateFutureState fields package-private.

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

@@ -392,17 +392,27 @@ void unpark() {
   /*
    * The following fields are package-private, even though we intend never to use them outside this
    * file. If they were instead private, then we wouldn't be able to access them reflectively from
-   * within VarHandleAtomicHelper.
+   * within VarHandleAtomicHelper and AtomicReferenceFieldUpdaterAtomicHelper.
    *
-   * Package-private "shouldn't" be necessary: VarHandleAtomicHelper and AbstractFutureState
-   * "should" be nestmates, so a call to MethodHandles.lookup inside VarHandleAtomicHelper "should"
-   * have access to AbstractFutureState's private fields. However, our open-source build uses
-   * `-source 8 -target 8`, so the class files from that build can't express nestmates. Thus, when
-   * those class files are used from Java 9 or higher (i.e., high enough to trigger the VarHandle
-   * code path), such a lookup would fail with an IllegalAccessException.
+   * Package-private "shouldn't" be necessary: The *AtomicHelper classes and AbstractFutureState
+   * "should" be nestmates, so a call to MethodHandles.lookup or
+   * AtomicReferenceFieldUpdater.newUpdater inside *AtomicHelper "should" have access to
+   * AbstractFutureState's private fields. However, our open-source build uses `-source 8 -target
+   * 8`, so the class files from that build can't express nestmates. Thus, when those class files
+   * are used from Java 9 or higher (i.e., high enough to trigger the VarHandle code path), such a
+   * lookup would fail with an IllegalAccessException. That may then trigger use of Unsafe (possibly
+   * with a warning under recent JVMs), or it may fall back even further to
+   * AtomicReferenceFieldUpdaterAtomicHelper, which would fail with a similar problem to
+   * VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedAtomicHelper.
    *
-   * This same problem is one of the reasons for us to likewise keep the fields in Waiter as
-   * package-private instead of private.
+   * Additionally, it seems that nestmates do not help with runtime reflection under *Android*, even
+   * when we use a newer -source and -target. That doesn't normally matter for AbstractFutureState,
+   * since Android should normally succed in using UnsafeAtomicHelper and thus never even try the
+   * problematic AtomicReferenceFieldUpdaterAtomicHelper code path. However, the same problem *does*
+   * matter with AggregateFutureState, which does not have an Unsafe-based helper.
+   *
+   * This same problem is one of the reasons for us to likewise use package-private for the fields
+   * in Waiter.
    */
 
   /**

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

@@ -41,11 +41,16 @@
 @ReflectionSupport(value = ReflectionSupport.Level.FULL)
 abstract class AggregateFutureState<OutputT extends @Nullable Object>
     extends AbstractFuture.TrustedFuture<OutputT> {
+  /*
+   * The following fields are package-private, even though we intend never to use them outside this
+   * file. For discussion, see AbstractFutureState.
+   */
+
   // Lazily initialized the first time we see an exception; not released until all the input futures
   // have completed and we have processed them all.
-  private volatile @Nullable Set<Throwable> seenExceptions = null;
+  volatile @Nullable Set<Throwable> seenExceptionsField = null;
 
-  private volatile int remaining;
+  volatile int remainingField;
 
   private static final AtomicHelper ATOMIC_HELPER;
 
@@ -73,27 +78,29 @@ abstract class AggregateFutureState<OutputT extends @Nullable Object>
   }
 
   AggregateFutureState(int remainingFutures) {
-    this.remaining = remainingFutures;
+    this.remainingField = remainingFutures;
   }
 
   final Set<Throwable> getOrInitSeenExceptions() {
     /*
-     * The initialization of seenExceptions has to be more complicated than we'd like. The simple
-     * approach would be for each caller CAS it from null to a Set populated with its exception. But
-     * there's another race: If the first thread fails with an exception and a second thread
-     * immediately fails with the same exception:
+     * The initialization of seenExceptionsField has to be more complicated than we'd like. The
+     * simple approach would be for each caller CAS it from null to a Set populated with its
+     * exception. But there's another race: If the first thread fails with an exception and a second
+     * thread immediately fails with the same exception:
      *
      * Thread1: calls setException(), which returns true, context switch before it can CAS
-     * seenExceptions to its exception
+     * seenExceptionsField to its exception
      *
-     * Thread2: calls setException(), which returns false, CASes seenExceptions to its exception,
-     * and wrongly believes that its exception is new (leading it to logging it when it shouldn't)
+     * Thread2: calls setException(), which returns false, CASes seenExceptionsField to its
+     * exception, and wrongly believes that its exception is new (leading it to logging it when it
+     * shouldn't)
      *
-     * Our solution is for threads to CAS seenExceptions from null to a Set populated with _the
-     * initial exception_, no matter which thread does the work. This ensures that seenExceptions
-     * always contains not just the current thread's exception but also the initial thread's.
+     * Our solution is for threads to CAS seenExceptionsField from null to a Set populated with _the
+     * initial exception_, no matter which thread does the work. This ensures that
+     * seenExceptionsField always contains not just the current thread's exception but also the
+     * initial thread's.
      */
-    Set<Throwable> seenExceptionsLocal = seenExceptions;
+    Set<Throwable> seenExceptionsLocal = seenExceptionsField;
     if (seenExceptionsLocal == null) {
       // TODO(cpovirk): Should we use a simpler (presumably cheaper) data structure?
       /*
@@ -128,7 +135,7 @@ final Set<Throwable> getOrInitSeenExceptions() {
        * requireNonNull is safe because either our compareAndSet succeeded or it failed because
        * another thread did it for us.
        */
-      seenExceptionsLocal = requireNonNull(seenExceptions);
+      seenExceptionsLocal = requireNonNull(seenExceptionsField);
     }
     return seenExceptionsLocal;
   }
@@ -141,7 +148,7 @@ final int decrementRemainingAndGet() {
   }
 
   final void clearSeenExceptions() {
-    seenExceptions = null;
+    seenExceptionsField = null;
   }
 
   @VisibleForTesting
@@ -150,11 +157,11 @@ static String atomicHelperTypeForTest() {
   }
 
   private abstract static class AtomicHelper {
-    /** Atomic compare-and-set of the {@link AggregateFutureState#seenExceptions} field. */
+    /** Performs an atomic compare-and-set of {@link AggregateFutureState#seenExceptionsField}. */
     abstract void compareAndSetSeenExceptions(
         AggregateFutureState<?> state, @Nullable Set<Throwable> expect, Set<Throwable> update);
 
-    /** Atomic decrement-and-get of the {@link AggregateFutureState#remaining} field. */
+    /** Performs an atomic decrement-and-get of {@link AggregateFutureState#remainingField}. */
     abstract int decrementAndGetRemainingCount(AggregateFutureState<?> state);
 
     abstract String atomicHelperTypeForTest();
@@ -163,10 +170,11 @@ abstract void compareAndSetSeenExceptions(
   private static final class SafeAtomicHelper extends AtomicHelper {
     private static final AtomicReferenceFieldUpdater<
             ? super AggregateFutureState<?>, ? super @Nullable Set<Throwable>>
-        seenExceptionsUpdater = seenExceptionsUpdaterFromWithinAggregateFutureState();
+        seenExceptionsUpdater =
+            newUpdater(AggregateFutureState.class, Set.class, "seenExceptionsField");
 
     private static final AtomicIntegerFieldUpdater<? super AggregateFutureState<?>>
-        remainingCountUpdater = remainingCountUpdaterFromWithinAggregateFutureState();
+        remainingCountUpdater = newUpdater(AggregateFutureState.class, "remainingField");
 
     @Override
     void compareAndSetSeenExceptions(
@@ -190,16 +198,16 @@ private static final class SynchronizedAtomicHelper extends AtomicHelper {
     void compareAndSetSeenExceptions(
         AggregateFutureState<?> state, @Nullable Set<Throwable> expect, Set<Throwable> update) {
       synchronized (state) {
-        if (state.seenExceptions == expect) {
-          state.seenExceptions = update;
+        if (state.seenExceptionsField == expect) {
+          state.seenExceptionsField = update;
         }
       }
     }
 
     @Override
     int decrementAndGetRemainingCount(AggregateFutureState<?> state) {
       synchronized (state) {
-        return --state.remaining;
+        return --state.remainingField;
       }
     }
 
@@ -208,27 +216,4 @@ String atomicHelperTypeForTest() {
       return "SynchronizedAtomicHelper";
     }
   }
-
-  /**
-   * Returns an {@link AtomicReferenceFieldUpdater} for {@link #seenExceptions}.
-   *
-   * <p>The creation of the updater has to happen directly inside {@link AggregateFutureState}, as
-   * discussed in {@link AbstractFuture#methodHandlesLookupFromWithinAbstractFuture}.
-   */
-  private static AtomicReferenceFieldUpdater<
-          ? super AggregateFutureState<?>, ? super @Nullable Set<Throwable>>
-      seenExceptionsUpdaterFromWithinAggregateFutureState() {
-    return newUpdater(AggregateFutureState.class, Set.class, "seenExceptions");
-  }
-
-  /**
-   * Returns an {@link AtomicIntegerFieldUpdater} for {@link #remaining}.
-   *
-   * <p>The creation of the updater has to happen directly inside {@link AggregateFutureState}, as
-   * discussed in {@link AbstractFuture#methodHandlesLookupFromWithinAbstractFuture}.
-   */
-  private static AtomicIntegerFieldUpdater<? super AggregateFutureState<?>>
-      remainingCountUpdaterFromWithinAggregateFutureState() {
-    return newUpdater(AggregateFutureState.class, "remaining");
-  }
 }

guava/src/com/google/common/util/concurrent/AbstractFutureState.java

@@ -392,17 +392,27 @@ void unpark() {
   /*
    * The following fields are package-private, even though we intend never to use them outside this
    * file. If they were instead private, then we wouldn't be able to access them reflectively from
-   * within VarHandleAtomicHelper.
+   * within VarHandleAtomicHelper and AtomicReferenceFieldUpdaterAtomicHelper.
    *
-   * Package-private "shouldn't" be necessary: VarHandleAtomicHelper and AbstractFutureState
-   * "should" be nestmates, so a call to MethodHandles.lookup inside VarHandleAtomicHelper "should"
-   * have access to AbstractFutureState's private fields. However, our open-source build uses
-   * `-source 8 -target 8`, so the class files from that build can't express nestmates. Thus, when
-   * those class files are used from Java 9 or higher (i.e., high enough to trigger the VarHandle
-   * code path), such a lookup would fail with an IllegalAccessException.
+   * Package-private "shouldn't" be necessary: The *AtomicHelper classes and AbstractFutureState
+   * "should" be nestmates, so a call to MethodHandles.lookup or
+   * AtomicReferenceFieldUpdater.newUpdater inside *AtomicHelper "should" have access to
+   * AbstractFutureState's private fields. However, our open-source build uses `-source 8 -target
+   * 8`, so the class files from that build can't express nestmates. Thus, when those class files
+   * are used from Java 9 or higher (i.e., high enough to trigger the VarHandle code path), such a
+   * lookup would fail with an IllegalAccessException. That may then trigger use of Unsafe (possibly
+   * with a warning under recent JVMs), or it may fall back even further to
+   * AtomicReferenceFieldUpdaterAtomicHelper, which would fail with a similar problem to
+   * VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedAtomicHelper.
    *
-   * This same problem is one of the reasons for us to likewise keep the fields in Waiter as
-   * package-private instead of private.
+   * Additionally, it seems that nestmates do not help with runtime reflection under *Android*, even
+   * when we use a newer -source and -target. That doesn't normally matter for AbstractFutureState,
+   * since Android should normally succed in using UnsafeAtomicHelper and thus never even try the
+   * problematic AtomicReferenceFieldUpdaterAtomicHelper code path. However, the same problem *does*
+   * matter with AggregateFutureState, which does not have an Unsafe-based helper.
+   *
+   * This same problem is one of the reasons for us to likewise use package-private for the fields
+   * in Waiter.
    */
 
   /**

guava/src/com/google/common/util/concurrent/AggregateFutureState.java

@@ -41,11 +41,16 @@
 @ReflectionSupport(value = ReflectionSupport.Level.FULL)
 abstract class AggregateFutureState<OutputT extends @Nullable Object>
     extends AbstractFuture.TrustedFuture<OutputT> {
+  /*
+   * The following fields are package-private, even though we intend never to use them outside this
+   * file. For discussion, see AbstractFutureState.
+   */
+
   // Lazily initialized the first time we see an exception; not released until all the input futures
   // have completed and we have processed them all.
-  private volatile @Nullable Set<Throwable> seenExceptions = null;
+  volatile @Nullable Set<Throwable> seenExceptionsField = null;
 
-  private volatile int remaining;
+  volatile int remainingField;
 
   private static final AtomicHelper ATOMIC_HELPER;
 
@@ -73,27 +78,29 @@ abstract class AggregateFutureState<OutputT extends @Nullable Object>
   }
 
   AggregateFutureState(int remainingFutures) {
-    this.remaining = remainingFutures;
+    this.remainingField = remainingFutures;
   }
 
   final Set<Throwable> getOrInitSeenExceptions() {
     /*
-     * The initialization of seenExceptions has to be more complicated than we'd like. The simple
-     * approach would be for each caller CAS it from null to a Set populated with its exception. But
-     * there's another race: If the first thread fails with an exception and a second thread
-     * immediately fails with the same exception:
+     * The initialization of seenExceptionsField has to be more complicated than we'd like. The
+     * simple approach would be for each caller CAS it from null to a Set populated with its
+     * exception. But there's another race: If the first thread fails with an exception and a second
+     * thread immediately fails with the same exception:
      *
      * Thread1: calls setException(), which returns true, context switch before it can CAS
-     * seenExceptions to its exception
+     * seenExceptionsField to its exception
      *
-     * Thread2: calls setException(), which returns false, CASes seenExceptions to its exception,
-     * and wrongly believes that its exception is new (leading it to logging it when it shouldn't)
+     * Thread2: calls setException(), which returns false, CASes seenExceptionsField to its
+     * exception, and wrongly believes that its exception is new (leading it to logging it when it
+     * shouldn't)
      *
-     * Our solution is for threads to CAS seenExceptions from null to a Set populated with _the
-     * initial exception_, no matter which thread does the work. This ensures that seenExceptions
-     * always contains not just the current thread's exception but also the initial thread's.
+     * Our solution is for threads to CAS seenExceptionsField from null to a Set populated with _the
+     * initial exception_, no matter which thread does the work. This ensures that
+     * seenExceptionsField always contains not just the current thread's exception but also the
+     * initial thread's.
      */
-    Set<Throwable> seenExceptionsLocal = seenExceptions;
+    Set<Throwable> seenExceptionsLocal = seenExceptionsField;
     if (seenExceptionsLocal == null) {
       // TODO(cpovirk): Should we use a simpler (presumably cheaper) data structure?
       /*
@@ -128,7 +135,7 @@ final Set<Throwable> getOrInitSeenExceptions() {
        * requireNonNull is safe because either our compareAndSet succeeded or it failed because
        * another thread did it for us.
        */
-      seenExceptionsLocal = requireNonNull(seenExceptions);
+      seenExceptionsLocal = requireNonNull(seenExceptionsField);
     }
     return seenExceptionsLocal;
   }
@@ -141,7 +148,7 @@ final int decrementRemainingAndGet() {
   }
 
   final void clearSeenExceptions() {
-    seenExceptions = null;
+    seenExceptionsField = null;
   }
 
   @VisibleForTesting
@@ -150,11 +157,11 @@ static String atomicHelperTypeForTest() {
   }
 
   private abstract static class AtomicHelper {
-    /** Atomic compare-and-set of the {@link AggregateFutureState#seenExceptions} field. */
+    /** Performs an atomic compare-and-set of {@link AggregateFutureState#seenExceptionsField}. */
     abstract void compareAndSetSeenExceptions(
         AggregateFutureState<?> state, @Nullable Set<Throwable> expect, Set<Throwable> update);
 
-    /** Atomic decrement-and-get of the {@link AggregateFutureState#remaining} field. */
+    /** Performs an atomic decrement-and-get of {@link AggregateFutureState#remainingField}. */
     abstract int decrementAndGetRemainingCount(AggregateFutureState<?> state);
 
     abstract String atomicHelperTypeForTest();
@@ -163,10 +170,11 @@ abstract void compareAndSetSeenExceptions(
   private static final class SafeAtomicHelper extends AtomicHelper {
     private static final AtomicReferenceFieldUpdater<
             ? super AggregateFutureState<?>, ? super @Nullable Set<Throwable>>
-        seenExceptionsUpdater = seenExceptionsUpdaterFromWithinAggregateFutureState();
+        seenExceptionsUpdater =
+            newUpdater(AggregateFutureState.class, Set.class, "seenExceptionsField");
 
     private static final AtomicIntegerFieldUpdater<? super AggregateFutureState<?>>
-        remainingCountUpdater = remainingCountUpdaterFromWithinAggregateFutureState();
+        remainingCountUpdater = newUpdater(AggregateFutureState.class, "remainingField");
 
     @Override
     void compareAndSetSeenExceptions(
@@ -190,16 +198,16 @@ private static final class SynchronizedAtomicHelper extends AtomicHelper {
     void compareAndSetSeenExceptions(
         AggregateFutureState<?> state, @Nullable Set<Throwable> expect, Set<Throwable> update) {
       synchronized (state) {
-        if (state.seenExceptions == expect) {
-          state.seenExceptions = update;
+        if (state.seenExceptionsField == expect) {
+          state.seenExceptionsField = update;
         }
       }
     }
 
     @Override
     int decrementAndGetRemainingCount(AggregateFutureState<?> state) {
       synchronized (state) {
-        return --state.remaining;
+        return --state.remainingField;
       }
     }
 
@@ -208,27 +216,4 @@ String atomicHelperTypeForTest() {
       return "SynchronizedAtomicHelper";
     }
   }
-
-  /**
-   * Returns an {@link AtomicReferenceFieldUpdater} for {@link #seenExceptions}.
-   *
-   * <p>The creation of the updater has to happen directly inside {@link AggregateFutureState}, as
-   * discussed in {@link AbstractFuture#methodHandlesLookupFromWithinAbstractFuture}.
-   */
-  private static AtomicReferenceFieldUpdater<
-          ? super AggregateFutureState<?>, ? super @Nullable Set<Throwable>>
-      seenExceptionsUpdaterFromWithinAggregateFutureState() {
-    return newUpdater(AggregateFutureState.class, Set.class, "seenExceptions");
-  }
-
-  /**
-   * Returns an {@link AtomicIntegerFieldUpdater} for {@link #remaining}.
-   *
-   * <p>The creation of the updater has to happen directly inside {@link AggregateFutureState}, as
-   * discussed in {@link AbstractFuture#methodHandlesLookupFromWithinAbstractFuture}.
-   */
-  private static AtomicIntegerFieldUpdater<? super AggregateFutureState<?>>
-      remainingCountUpdaterFromWithinAggregateFutureState() {
-    return newUpdater(AggregateFutureState.class, "remaining");
-  }
 }

proguard/concurrent.pro

@@ -20,8 +20,8 @@
   *** value;
 }
 -keepclassmembers class com.google.common.util.concurrent.AggregateFutureState {
-  *** remaining;
-  *** seenExceptions;
+  *** remainingField;
+  *** seenExceptionsField;
 }
 
 # Since Unsafe is using the field offsets of these inner classes, we don't want