Use AtomicReferenceFieldUpdater instead of VarHandle when guava-android runs under a JVM.

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

@@ -30,10 +30,11 @@
 @NullUnmarked
 public class AbstractFutureDefaultAtomicHelperTest extends TestCase {
   public void testUsingExpectedAtomicHelper() throws Exception {
-    if (isJava8() || isAndroid()) {
+    if (isAndroid()) {
       assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("UnsafeAtomicHelper");
     } else {
-      assertThat(AbstractFutureState.atomicHelperTypeForTest()).isEqualTo("VarHandleAtomicHelper");
+      assertThat(AbstractFutureState.atomicHelperTypeForTest())
+          .isEqualTo("AtomicReferenceFieldUpdaterAtomicHelper");
     }
   }
 

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

@@ -89,26 +89,46 @@ public static TestSuite suite() {
 
   @Override
   public void runTest() throws Exception {
-    // First ensure that our classloaders are initializing the correct helper versions
-    if (isJava8() || isAndroid()) {
-      checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
-    } else {
-      checkHelperVersion(getClass().getClassLoader(), "VarHandleAtomicHelper");
-    }
-    checkHelperVersion(NO_VAR_HANDLE, "UnsafeAtomicHelper");
+    /*
+     * Note that we do not run this test under Android at the moment. For Android testing, see
+     * AbstractFutureDefaultAtomicHelperTest.
+     */
+
+    // First, ensure that our classloaders are initializing the correct helper versions:
+
+    checkHelperVersion(getClass().getClassLoader(), "AtomicReferenceFieldUpdaterAtomicHelper");
+    /*
+     * Since we use AtomicReferenceFieldUpdaterAtomicHelper by default, we'll "obviously" use it
+     * even when Unsafe isn't available. But it's nice to have a check here to make sure that
+     * nothing somehow goes wrong as the JDK restricts access to Unsafe.
+     */
     checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper");
+    /*
+     * SynchronizedHelper is meant for Android, but our best way to test it is under the JVM.
+     *
+     * SynchronizedHelper also ends up getting used under the JVM during
+     * AggregateFutureStateFallbackAtomicHelperTest, as discussed in a comment in
+     * AggregateFutureState.
+     *
+     * Here, we check that we're able to force AbstractFutureState to select SynchronizedHelper, and
+     * below, we actually run the AbstractFutureTest methods under that scenario.
+     */
     checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper");
 
+    // Then, run the actual tests under each alternative classloader:
+
     /*
-     * Under Java 8 or Android, there is no need to test the no-VarHandle case here: It's already
-     * tested by the main AbstractFutureTest, which uses the default AtomicHelper, which we verified
-     * above to be UnsafeAtomicHelper.
+     * We don't need to test further under NO_UNSAFE: We verified that it selects
+     * AtomicReferenceFieldUpdaterAtomicHelper, which is the default, which means that it's used
+     * when we run AbstractFutureTest itself.
      */
-    if (!isJava8() && !isAndroid()) {
-      runTestMethod(NO_VAR_HANDLE);
-    }
 
-    runTestMethod(NO_UNSAFE);
+    /*
+     * We don't test UnsafeAtomicHelper here, since guava-android doesn't provide a way to use it
+     * under the JVM. (We could arrange for one if we really wanted, but that will break once the
+     * JDK further restricts access to Unsafe.) But we have coverage under an Android emulator,
+     * which uses UnsafeAtomicHelper when it runs AbstractFutureTest itself.
+     */
 
     runTestMethod(NO_ATOMIC_REFERENCE_FIELD_UPDATER);
     // TODO(lukes): assert that the logs are full of errors
@@ -164,8 +184,4 @@ public Class<?> loadClass(String name) throws ClassNotFoundException {
   private static boolean isJava8() {
     return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
   }
-
-  private static boolean isAndroid() {
-    return System.getProperty("java.runtime.name", "").contains("Android");
-  }
 }

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

@@ -86,10 +86,18 @@ public static TestSuite suite() {
 
   @Override
   public void runTest() throws Exception {
-    // First ensure that our classloaders are initializing the correct helper versions
+    /*
+     * Note that we do not run this test under Android at the moment. For Android testing, see
+     * AggregateFutureStateDefaultAtomicHelperTest.
+     */
+
+    // First, ensure that our classloaders are initializing the correct helper versions:
+
     checkHelperVersion(getClass().getClassLoader(), "SafeAtomicHelper");
     checkHelperVersion(NO_ATOMIC_FIELD_UPDATER, "SynchronizedAtomicHelper");
 
+    // Then, run the actual tests under each alternative classloader:
+
     runTestMethod(NO_ATOMIC_FIELD_UPDATER);
     // TODO(lukes): assert that the logs are full of errors
   }

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

@@ -27,11 +27,8 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.util.concurrent.AbstractFuture.Listener;
 import com.google.common.util.concurrent.internal.InternalFutureFailureAccess;
-import com.google.j2objc.annotations.J2ObjCIncompatible;
 import com.google.j2objc.annotations.ReflectionSupport;
 import com.google.j2objc.annotations.RetainedLocalRef;
-import java.lang.invoke.MethodHandles;
-import java.lang.invoke.VarHandle;
 import java.lang.reflect.Field;
 import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
@@ -348,8 +345,7 @@ void unpark() {
     Throwable thrownUnsafeFailure = null;
     Throwable thrownAtomicReferenceFieldUpdaterFailure = null;
 
-    helper = VarHandleAtomicHelperMaker.INSTANCE.tryMakeVarHandleAtomicHelper();
-    if (helper == null) {
+    if (mightBeAndroid()) {
       try {
         helper = new UnsafeAtomicHelper();
       } catch (Exception | Error unsafeFailure) { // sneaky checked exception
@@ -367,6 +363,39 @@ void unpark() {
           helper = new SynchronizedHelper();
         }
       }
+    } else {
+      /*
+       * We avoid Unsafe, since newer JVMs produce warnings or even errors for attempts to use it.
+       *
+       * In guava-jre, we avoid Unsafe by using VarHandle instead. But if we have references to
+       * VarHandle in guava-android, even if they're unused under Android, we cause errors under
+       * AGP: https://github.com/google/guava/issues/7769.
+       *
+       * My impression is that an AtomicReferenceFieldUpdater in a static field is similarly fast to
+       * Unsafe on modern JVMs (if perhaps not quite as fast as VarHandle?). However, I'm not sure
+       * exactly what we've benchmarked, and we certainly haven't benchmarked as far back as JDK 8.
+       * (We also haven't benchmarked under Android. We continue to use UnsafeAtomicHelper there so
+       * that we don't change the performance there, for better or for worse.) Fortunately, JVM
+       * users will typically use guava-jre, not guava-android, and guava-jre uses the VarHandle
+       * implementation when possible.
+       */
+      try {
+        helper = new AtomicReferenceFieldUpdaterAtomicHelper();
+      } catch (NoClassDefFoundError fromAggregateFutureStateFallbackAtomicHelperTest) {
+        /*
+         * AtomicReferenceFieldUpdaterAtomicHelper should always work on the JVM. (I mean, it
+         * "should" always work on Android, too, but we know of a Samsung bug there :)) However, in
+         * AggregateFutureStateFallbackAtomicHelperTest, we test what happens to AggregateFuture in
+         * the case of the Samsung bug, and we do that by breaking AtomicReferenceFieldUpdater.
+         * Breaking AtomicReferenceFieldUpdater not only forces AggregateFutureState to fall back to
+         * another implementation but also forces AbstractFutureState to be able to do the
+         * same—hence the try-catch here.
+         *
+         * (Really, we're fortunate that breaking AtomicReferenceFieldUpdater doesn't break _even
+         * more_ things.)
+         */
+        helper = new SynchronizedHelper();
+      }
     }
     ATOMIC_HELPER = helper;
 
@@ -403,7 +432,7 @@ void unpark() {
    * 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.
+   * VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedHelper.
    *
    * 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,
@@ -512,43 +541,6 @@ static String atomicHelperTypeForTest() {
     return ATOMIC_HELPER.atomicHelperTypeForTest();
   }
 
-  private enum VarHandleAtomicHelperMaker {
-    INSTANCE {
-      /**
-       * Implementation used by non-J2ObjC environments (aside, of course, from those that have
-       * supersource for the entirety of {@link AbstractFutureState}).
-       */
-      @Override
-      @J2ObjCIncompatible
-      @Nullable AtomicHelper tryMakeVarHandleAtomicHelper() {
-        if (mightBeAndroid()) {
-          return null;
-        }
-        try {
-          /*
-           * We first use reflection to check whether VarHandle exists. If we instead just tried to
-           * load our class directly (which would trigger non-reflective loading of VarHandle) from
-           * within a `try` block, then an error might be thrown even before we enter the `try`
-           * block: https://github.com/google/truth/issues/333#issuecomment-765652454
-           *
-           * Also, it's nice that this approach should let us catch *only* ClassNotFoundException
-           * instead of having to catch more broadly (potentially even including, say, a
-           * StackOverflowError).
-           */
-          Class.forName("java.lang.invoke.VarHandle");
-        } catch (ClassNotFoundException beforeJava9) {
-          return null;
-        }
-        return new VarHandleAtomicHelper();
-      }
-    };
-
-    /** Implementation used by J2ObjC environments, overridden for other environments. */
-    @Nullable AtomicHelper tryMakeVarHandleAtomicHelper() {
-      return null;
-    }
-  }
-
   private abstract static class AtomicHelper {
     /** Non-volatile write of the thread to the {@link Waiter#thread} field. */
     abstract void putThread(Waiter waiter, Thread newValue);
@@ -577,81 +569,6 @@ abstract boolean casValue(
     abstract String atomicHelperTypeForTest();
   }
 
-  /** {@link AtomicHelper} based on {@link VarHandle}. */
-  @J2ObjCIncompatible
-  // We use this class only after confirming that VarHandle is available at runtime.
-  @SuppressWarnings({"Java8ApiChecker", "Java7ApiChecker", "AndroidJdkLibsChecker"})
-  @IgnoreJRERequirement
-  private static final class VarHandleAtomicHelper extends AtomicHelper {
-    static final VarHandle waiterThreadUpdater;
-    static final VarHandle waiterNextUpdater;
-    static final VarHandle waitersUpdater;
-    static final VarHandle listenersUpdater;
-    static final VarHandle valueUpdater;
-
-    static {
-      MethodHandles.Lookup lookup = MethodHandles.lookup();
-      try {
-        waiterThreadUpdater = lookup.findVarHandle(Waiter.class, "thread", Thread.class);
-        waiterNextUpdater = lookup.findVarHandle(Waiter.class, "next", Waiter.class);
-        waitersUpdater =
-            lookup.findVarHandle(AbstractFutureState.class, "waitersField", Waiter.class);
-        listenersUpdater =
-            lookup.findVarHandle(AbstractFutureState.class, "listenersField", Listener.class);
-        valueUpdater = lookup.findVarHandle(AbstractFutureState.class, "valueField", Object.class);
-      } catch (ReflectiveOperationException e) {
-        // Those fields exist.
-        throw newLinkageError(e);
-      }
-    }
-
-    @Override
-    void putThread(Waiter waiter, Thread newValue) {
-      waiterThreadUpdater.setRelease(waiter, newValue);
-    }
-
-    @Override
-    void putNext(Waiter waiter, @Nullable Waiter newValue) {
-      waiterNextUpdater.setRelease(waiter, newValue);
-    }
-
-    @Override
-    boolean casWaiters(
-        AbstractFutureState<?> future, @Nullable Waiter expect, @Nullable Waiter update) {
-      return waitersUpdater.compareAndSet(future, expect, update);
-    }
-
-    @Override
-    boolean casListeners(
-        AbstractFutureState<?> future, @Nullable Listener expect, Listener update) {
-      return listenersUpdater.compareAndSet(future, expect, update);
-    }
-
-    @Override
-    @Nullable Listener gasListeners(AbstractFutureState<?> future, Listener update) {
-      return (Listener) listenersUpdater.getAndSet(future, update);
-    }
-
-    @Override
-    @Nullable Waiter gasWaiters(AbstractFutureState<?> future, Waiter update) {
-      return (Waiter) waitersUpdater.getAndSet(future, update);
-    }
-
-    @Override
-    boolean casValue(AbstractFutureState<?> future, @Nullable Object expect, Object update) {
-      return valueUpdater.compareAndSet(future, expect, update);
-    }
-
-    private static LinkageError newLinkageError(Throwable cause) {
-      return new LinkageError(cause.toString(), cause);
-    }
-
-    @Override
-    String atomicHelperTypeForTest() {
-      return "VarHandleAtomicHelper";
-    }
-  }
-
   /**
    * {@link AtomicHelper} based on {@link sun.misc.Unsafe}.
    *

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

@@ -89,8 +89,14 @@ public static TestSuite suite() {
 
   @Override
   public void runTest() throws Exception {
-    // First ensure that our classloaders are initializing the correct helper versions
-    if (isJava8() || isAndroid()) {
+    /*
+     * Note that we do not run this test under Android at the moment. For Android testing, see
+     * AbstractFutureDefaultAtomicHelperTest.
+     */
+
+    // First, ensure that our classloaders are initializing the correct helper versions:
+
+    if (isJava8()) {
       checkHelperVersion(getClass().getClassLoader(), "UnsafeAtomicHelper");
     } else {
       checkHelperVersion(getClass().getClassLoader(), "VarHandleAtomicHelper");
@@ -99,12 +105,14 @@ public void runTest() throws Exception {
     checkHelperVersion(NO_UNSAFE, "AtomicReferenceFieldUpdaterAtomicHelper");
     checkHelperVersion(NO_ATOMIC_REFERENCE_FIELD_UPDATER, "SynchronizedHelper");
 
+    // Then, run the actual tests under each alternative classloader:
+
     /*
-     * Under Java 8 or Android, there is no need to test the no-VarHandle case here: It's already
-     * tested by the main AbstractFutureTest, which uses the default AtomicHelper, which we verified
-     * above to be UnsafeAtomicHelper.
+     * Under Java 8, there is no need to test the no-VarHandle case here: It's already tested by the
+     * main AbstractFutureTest, which uses the default AtomicHelper, which we verified above to be
+     * UnsafeAtomicHelper.
      */
-    if (!isJava8() && !isAndroid()) {
+    if (!isJava8()) {
       runTestMethod(NO_VAR_HANDLE);
     }
 
@@ -164,8 +172,4 @@ public Class<?> loadClass(String name) throws ClassNotFoundException {
   private static boolean isJava8() {
     return JAVA_SPECIFICATION_VERSION.value().equals("1.8");
   }
-
-  private static boolean isAndroid() {
-    return System.getProperty("java.runtime.name", "").contains("Android");
-  }
 }

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

@@ -86,10 +86,18 @@ public static TestSuite suite() {
 
   @Override
   public void runTest() throws Exception {
-    // First ensure that our classloaders are initializing the correct helper versions
+    /*
+     * Note that we do not run this test under Android at the moment. For Android testing, see
+     * AggregateFutureStateDefaultAtomicHelperTest.
+     */
+
+    // First, ensure that our classloaders are initializing the correct helper versions:
+
     checkHelperVersion(getClass().getClassLoader(), "SafeAtomicHelper");
     checkHelperVersion(NO_ATOMIC_FIELD_UPDATER, "SynchronizedAtomicHelper");
 
+    // Then, run the actual tests under each alternative classloader:
+
     runTestMethod(NO_ATOMIC_FIELD_UPDATER);
     // TODO(lukes): assert that the logs are full of errors
   }

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

@@ -403,7 +403,7 @@ void unpark() {
    * 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.
+   * VarHandleAtomicHelperMaker, forcing us all the way to SynchronizedHelper.
    *
    * 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,

proguard/concurrent.pro

@@ -47,10 +47,3 @@
 -keep class com.google.apphosting.api.ApiProxy {
   static *** getCurrentEnvironment (...);
 }
-
-# b/407533570
--dontwarn com.google.common.util.concurrent.AbstractFutureState$VarHandleAtomicHelper
-# See post-submit commentary on cl/743198569 about b/408047495.
--assumevalues class com.google.common.util.concurrent.AbstractFutureState {
-  boolean mightBeAndroid() return true;
-}