Make FileBackedOutputStream use PhantomReference instead of finalize().

This generally makes it more resilient and future-proof, but we still don't recommend relying on GC for object cleanup, in part because any small change (like this one) could upset a delicate balance somewhere.

Also, while I'm messing with `reachabilityFence` again: Remove a confused comment from 7eba58f in `GcFinalizationTest`: Once we use `reachabilityFence` directly, we'll eliminate the helper method, and http://errorprone.info/bugpattern/ReachabilityFenceUsage will remain happy because the call to `reachabilityFence` will appear directly inside a `finally` block. (I'm still not sure that `finally` is necessary there, but it's easy, and it's probably reasonable as a default practice.)

(I'm also not sure if `finally` is necessary for the new usages of `reachabilityFence` in `FileBackedOutputStream`. I guess the fear would be that we'd allow the `FileOutputStream` to be closed while it's in the process of preparing to throw an exception, and the `close` call could clear something that the exception-construction code wants to read? I do suspect that the new `reachabilityFence` calls are a good idea to have _somewhere_, whether in a `finally` block or not, to ensure that background cleanup doesn't `reset` a `FileBackedOutputStream` during a call to a method like `write` (which might then open a _new_ file) or `close` (which might then not see an exception because the background `reset` sees it instead). Or maybe [`reachabilityFence` isn't necessary because our methods are `synchronized`](https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/lang/ref/Reference.html#reachabilityFence(java.lang.Object)). (`synchronized` should at least mean that we don't need the fence in a `finally` block, at least not for the reasons that I mentioned above: It won't be possible to `reset` the stream until `write` or `close` exits the critical area.) But presumably it won't hurt anything.)

RELNOTES=n/a
PiperOrigin-RevId: 875720195

Author: cpovirk

android/guava-testlib/src/com/google/common/testing/GcFinalization.java

@@ -138,7 +138,7 @@ private static long timeoutSeconds() {
    *
    * @throws RuntimeException if timed out or interrupted while waiting
    */
-  @SuppressWarnings("removal") // b/260137033
+  @SuppressWarnings("removal") // b/487687332
   public static void awaitDone(Future<?> future) {
     if (future.isDone()) {
       return;
@@ -171,7 +171,7 @@ public static void awaitDone(Future<?> future) {
    *
    * @throws RuntimeException if timed out or interrupted while waiting
    */
-  @SuppressWarnings("removal") // b/260137033
+  @SuppressWarnings("removal") // b/487687332
   public static void awaitDone(FinalizationPredicate predicate) {
     if (predicate.isDone()) {
       return;
@@ -200,7 +200,7 @@ public static void awaitDone(FinalizationPredicate predicate) {
    *
    * @throws RuntimeException if timed out or interrupted while waiting
    */
-  @SuppressWarnings("removal") // b/260137033
+  @SuppressWarnings("removal") // b/487687332
   public static void await(CountDownLatch latch) {
     if (latch.getCount() == 0) {
       return;
@@ -232,7 +232,7 @@ public static void await(CountDownLatch latch) {
   private static void createUnreachableLatchFinalizer(CountDownLatch latch) {
     Object unused =
         new Object() {
-          @SuppressWarnings({"removal", "Finalize"}) // b/260137033
+          @SuppressWarnings({"removal", "Finalize"}) // b/487687332
           @Override
           protected void finalize() {
             latch.countDown();
@@ -298,7 +298,7 @@ public static void awaitClear(WeakReference<?> ref) {
    * @throws RuntimeException if timed out or interrupted while waiting
    * @since 12.0
    */
-  @SuppressWarnings({"removal", "Finalize"}) // b/260137033
+  @SuppressWarnings({"removal", "Finalize"}) // b/487687332
   public static void awaitFullGc() {
     CountDownLatch finalizerRan = new CountDownLatch(1);
     WeakReference<Object> ref =

android/guava-testlib/test/com/google/common/testing/GcFinalizationTest.java

@@ -16,15 +16,14 @@
 
 package com.google.common.testing;
 
-import static com.google.common.testing.SneakyThrows.sneakyThrow;
 import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertThrows;
 
-import com.google.common.testing.GcFinalization.FinalizationPredicate;
 import com.google.common.util.concurrent.SettableFuture;
 import java.lang.ref.Reference;
 import java.lang.ref.WeakReference;
 import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.util.WeakHashMap;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -51,7 +50,7 @@ public void testAwait_countDownLatch() {
     CountDownLatch latch = new CountDownLatch(1);
     Object unused =
         new Object() {
-          @SuppressWarnings({"removal", "Finalize"}) // b/260137033
+          @SuppressWarnings({"removal", "Finalize"}) // b/487687332
           @Override
           protected void finalize() {
             latch.countDown();
@@ -66,7 +65,7 @@ public void testAwaitDone_future() {
     SettableFuture<@Nullable Void> future = SettableFuture.create();
     Object unused =
         new Object() {
-          @SuppressWarnings({"removal", "Finalize"}) // b/260137033
+          @SuppressWarnings({"removal", "Finalize"}) // b/487687332
           @Override
           protected void finalize() {
             future.set(null);
@@ -82,7 +81,7 @@ public void testAwaitDone_future_cancel() {
     SettableFuture<@Nullable Void> future = SettableFuture.create();
     Object unused =
         new Object() {
-          @SuppressWarnings({"removal", "Finalize"}) // b/260137033
+          @SuppressWarnings({"removal", "Finalize"}) // b/487687332
           @Override
           protected void finalize() {
             future.cancel(false);
@@ -103,13 +102,7 @@ public void testAwaitClear() {
   public void testAwaitDone_finalizationPredicate() {
     WeakHashMap<Object, Object> map = new WeakHashMap<>();
     map.put(new Object(), Boolean.TRUE);
-    GcFinalization.awaitDone(
-        new FinalizationPredicate() {
-          @Override
-          public boolean isDone() {
-            return map.isEmpty();
-          }
-        });
+    GcFinalization.awaitDone(map::isEmpty);
     assertTrue(map.isEmpty());
   }
 
@@ -202,16 +195,7 @@ public void testAwaitDone_finalizationPredicate_interrupted() {
     Interruptenator interruptenator = new Interruptenator(Thread.currentThread());
     try {
       RuntimeException expected =
-          assertThrows(
-              RuntimeException.class,
-              () ->
-                  GcFinalization.awaitDone(
-                      new FinalizationPredicate() {
-                        @Override
-                        public boolean isDone() {
-                          return false;
-                        }
-                      }));
+          assertThrows(RuntimeException.class, () -> GcFinalization.awaitDone(() -> false));
       assertWrapsInterruptedException(expected);
     } finally {
       interruptenator.shutdown();
@@ -227,9 +211,9 @@ public boolean isDone() {
   public void testAwaitFullGc() {
     CountDownLatch finalizerRan = new CountDownLatch(1);
     WeakReference<Object> ref =
-        new WeakReference<Object>(
+        new WeakReference<>(
             new Object() {
-              @SuppressWarnings({"removal", "Finalize"}) // b/260137033
+              @SuppressWarnings({"removal", "Finalize"}) // b/487687332
               @Override
               protected void finalize() {
                 finalizerRan.countDown();
@@ -247,26 +231,35 @@ protected void finalize() {
     assertThat(ref.get()).isNull();
   }
 
-  /*
-   * Once we can call Reference.reachabilityFence() directly, we will need to suppress
-   * https://errorprone.info/bugpattern/ReachabilityFenceUsage. Suppressing is fine: This method is a wrapper around that
-   * method, and we call that wrapper from a `finally` block, as the check wants. (Also, it's not
-   * entirely clear to me that we really need to use `finally` in this case: b/127970178.)
-   */
-  private static void reachabilityFence(Object o) {
+  // We call the method only after checking that it's present.
+  @IgnoreJRERequirement
+  @SuppressWarnings({
+    "Java8ApiChecker",
+    // This method is a helper, which we call from a `finally` block, as recommended.
+    "ReachabilityFenceUsage",
+  })
+  static void reachabilityFence(@Nullable Object o) {
+    if (IS_REACHABILITY_FENCE_METHOD_USABLE) {
+      Reference.reachabilityFence(o);
+    }
+  }
+
+  private static final boolean IS_REACHABILITY_FENCE_METHOD_USABLE =
+      computeIsReachabilityFenceMethodUsable();
+
+  private static boolean computeIsReachabilityFenceMethodUsable() {
     try {
-      Reference.class.getMethod("reachabilityFence", Object.class).invoke(null, o);
-    } catch (InvocationTargetException e) {
-      // The method has no `throws` clause.
-      sneakyThrow(e.getCause());
-    } catch (IllegalAccessException e) {
+      Method method = Reference.class.getMethod("reachabilityFence", Object.class);
+      method.invoke(null, GcFinalizationTest.class); // to make sure the method is accessible
+      return true;
+    } catch (NoSuchMethodException | IllegalAccessException probablyBeforeJava9OrAndroid28) {
       /*
-       * The method should be public (though technically there's nothing preventing it from being
-       * present as non-public under older versions).
+       * It's theoretically possible for Reference.reachabilityFence to exist under older VMs in an
+       * inaccessible form.
        */
-      throw new LinkageError(e.toString(), e);
-    } catch (NoSuchMethodException e) {
-      // We're running under Java 8 or under Android before API Level 28.
+      return false;
+    } catch (InvocationTargetException e) {
+      throw new AssertionError(e.getCause());
     }
   }
 }

android/guava-testlib/test/com/google/common/testing/SneakyThrows.java

@@ -17,7 +17,10 @@
 package com.google.common.io;
 
 import static com.google.common.base.StandardSystemProperty.OS_NAME;
+import static com.google.common.io.FileBackedOutputStream.reachabilityFence;
 import static com.google.common.primitives.Bytes.concat;
+import static com.google.common.testing.GcFinalization.awaitClear;
+import static com.google.common.testing.GcFinalization.awaitDone;
 import static com.google.common.truth.Truth.assertThat;
 import static java.lang.Math.min;
 import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
@@ -27,6 +30,7 @@
 import java.io.File;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.lang.ref.WeakReference;
 import java.nio.file.attribute.PosixFileAttributeView;
 import java.nio.file.attribute.PosixFileAttributes;
 import org.jspecify.annotations.NullUnmarked;
@@ -54,10 +58,11 @@ public void testThreshold() throws Exception {
   }
 
   private void testThreshold(
-      int fileThreshold, int dataSize, boolean singleByte, boolean resetOnFinalize)
+      int fileThreshold, int dataSize, boolean singleByte, boolean resetWhenGarbageCollected)
       throws IOException {
     byte[] data = newPreFilledByteArray(dataSize);
-    FileBackedOutputStream out = new FileBackedOutputStream(fileThreshold, resetOnFinalize);
+    FileBackedOutputStream out =
+        new FileBackedOutputStream(fileThreshold, resetWhenGarbageCollected);
     ByteSource source = out.asByteSource();
     int chunk1 = min(dataSize, fileThreshold);
     int chunk2 = dataSize - chunk1;
@@ -97,7 +102,7 @@ private void testThreshold(
   }
 
 
-  public void testThreshold_resetOnFinalize() throws Exception {
+  public void testThreshold_resetWhenGarbageCollected() throws Exception {
     testThreshold(0, 100, true, true);
     testThreshold(10, 100, true, true);
     testThreshold(100, 100, true, true);
@@ -197,4 +202,20 @@ public void testThresholdCrossing_resourceManagement() throws Exception {
 
     out.reset();
   }
+
+
+  @AndroidIncompatible // depends on details of GC
+  public void testResetWhenGarbageCollected() throws Exception {
+    FileBackedOutputStream out =
+        new FileBackedOutputStream(0, /* resetWhenGarbageCollected= */ true);
+    out.write(new byte[10]);
+    File file = out.getFile();
+    assertThat(file.exists()).isTrue();
+    reachabilityFence(out); // not sure if this is necessary even in theory
+
+    WeakReference<ByteSource> sourceRef = new WeakReference<>(out.asByteSource());
+    out = null;
+    awaitClear(sourceRef);
+    awaitDone(() -> !file.exists());
+  }
 }