Make AggregateFutureState fields package-private.

This ensures that they are accessible to methods like `AtomicIntegerFieldUpdater.newUpdater` no matter which `util.concurrent` class those methods are called from.

Previously, we were trying to arrange for those methods to be called from `AggregateFutureState` itself—specifically, through methods like `remainingCountUpdaterFromWithinAggregateFutureState`. However, we're finding that optimizers sometimes inline those methods into callers in other classes, leading to the warning "SafeAtomicHelper is broken!"

Rather than try to prevent inlining with `-dontinline` directives, we instead give in and expand the fields' visibility. We already did this for `AbstractFutureState` in cl/742334547, albeit for somewhat different reasons, so we can follow the same playbook here: Rename the fields to make them harder to use by accident (as in cl/741607075), and update our Proguard config for the rename (as in cl/742724817). At that point, we might as well inline methods like `remainingCountUpdaterFromWithinAggregateFutureState` ourselves just to simplify the code, so I've done so.

(Note that even `private` fields "should" be accessible to nested classes, thanks to nestmates. However, that's [not the case with `-source 8 -target 8`](https://github.com/google/guava/blob/a429676a3bb68ac9b29cea0f15ae65065bdf5a44/guava/src/com/google/common/util/concurrent/AbstractFutureState.java#L397-L402), and apparently it's not the case for Android, as well, even without `-source 8 -target 8`.)

RELNOTES=`util.concurrent`: Modified our fast paths to ensure that they continue to work when run through optimizers, such as those commonly used by Android apps. This fixes problems that some users may have seen since [Guava 33.4.5](https://github.com/google/guava/releases/tag/v33.4.5). (b8dcaed)
PiperOrigin-RevId: 744855670

Author: cpovirk

Diff for: 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.
    */
 
   /**

Diff for: 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");
-  }
 }

Diff for: 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.
    */
 
   /**

Diff for: 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");
-  }
 }

Diff for: 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