From 62c61872053d639b6a1622797848c95badc8afab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=89amonn=20McManus?= <emcmanus@google.com>
Date: Thu, 12 Dec 2024 07:00:29 -0800
Subject: [PATCH] Recommend `Cleaner` instead of `FinalizableReferenceQueue`.

Also add a test that validates that the example given for FRQ in its doc comment are sound.

We're not deprecating FRQ yet, because `Cleaner` is still fairly recent in the Android world.

RELNOTES=We now recommend the standard `Cleaner` class instead of `FinalizableReferenceQueue`, when `Cleaner` is available.
PiperOrigin-RevId: 705490132
---
 .../base/FinalizableReferenceQueueTest.java   | 103 ++++++++++++++----
 .../base/FinalizableReferenceQueue.java       |  53 ++++++++-
 .../base/FinalizableReferenceQueueTest.java   | 103 ++++++++++++++----
 .../base/FinalizableReferenceQueue.java       |  53 ++++++++-
 4 files changed, 266 insertions(+), 46 deletions(-)

diff --git a/android/guava-tests/test/com/google/common/base/FinalizableReferenceQueueTest.java b/android/guava-tests/test/com/google/common/base/FinalizableReferenceQueueTest.java
index 5b2dce787572..db1288c4dca9 100644
--- a/android/guava-tests/test/com/google/common/base/FinalizableReferenceQueueTest.java
+++ b/android/guava-tests/test/com/google/common/base/FinalizableReferenceQueueTest.java
@@ -16,17 +16,28 @@
 
 package com.google.common.base;
 
+import static com.google.common.truth.Truth.assertThat;
+
 import com.google.common.annotations.GwtIncompatible;
 import com.google.common.base.internal.Finalizer;
+import com.google.common.collect.Sets;
 import com.google.common.testing.GcFinalization;
+import java.io.Closeable;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.ref.Reference;
 import java.lang.ref.ReferenceQueue;
 import java.lang.ref.WeakReference;
+import java.net.ServerSocket;
 import java.net.URL;
 import java.net.URLClassLoader;
-import java.util.Arrays;
-import java.util.Collections;
-import junit.framework.TestCase;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 import org.checkerframework.checker.nullness.qual.Nullable;
+import org.junit.After;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Unit test for {@link FinalizableReferenceQueue}.
@@ -38,26 +49,21 @@
 // - possibly no real concept of separate ClassLoaders?
 @AndroidIncompatible
 @GwtIncompatible
-public class FinalizableReferenceQueueTest extends TestCase {
+@RunWith(JUnit4.class)
+public class FinalizableReferenceQueueTest {
 
   private @Nullable FinalizableReferenceQueue frq;
 
-  @Override
-  protected void tearDown() throws Exception {
+  @After
+  public void tearDown() throws Exception {
     frq = null;
   }
 
-
+  @Test
   public void testFinalizeReferentCalled() {
     final MockReference reference = new MockReference(frq = new FinalizableReferenceQueue());
 
-    GcFinalization.awaitDone(
-        new GcFinalization.FinalizationPredicate() {
-          @Override
-          public boolean isDone() {
-            return reference.finalizeReferentCalled;
-          }
-        });
+    GcFinalization.awaitDone(() -> reference.finalizeReferentCalled);
   }
 
   static class MockReference extends FinalizableWeakReference<Object> {
@@ -80,7 +86,7 @@ public void finalizeReferent() {
    */
   private WeakReference<ReferenceQueue<Object>> queueReference;
 
-
+  @Test
   public void testThatFinalizerStops() {
     weaklyReferenceQueue();
     GcFinalization.awaitClear(queueReference);
@@ -109,6 +115,7 @@ public void finalizeReferent() {
         };
   }
 
+  @Test
   public void testDecoupledLoader() {
     FinalizableReferenceQueue.DecoupledLoader decoupledLoader =
         new FinalizableReferenceQueue.DecoupledLoader() {
@@ -120,10 +127,10 @@ URLClassLoader newLoader(URL base) {
 
     Class<?> finalizerCopy = decoupledLoader.loadFinalizer();
 
-    assertNotNull(finalizerCopy);
-    assertNotSame(Finalizer.class, finalizerCopy);
+    assertThat(finalizerCopy).isNotNull();
+    assertThat(finalizerCopy).isNotSameInstanceAs(Finalizer.class);
 
-    assertNotNull(FinalizableReferenceQueue.getStartFinalizer(finalizerCopy));
+    assertThat(FinalizableReferenceQueue.getStartFinalizer(finalizerCopy)).isNotNull();
   }
 
   static class DecoupledClassLoader extends URLClassLoader {
@@ -148,13 +155,69 @@ protected synchronized Class<?> loadClass(String name, boolean resolve)
     }
   }
 
+  @Test
   public void testGetFinalizerUrl() {
-    assertNotNull(getClass().getResource("internal/Finalizer.class"));
+    assertThat(getClass().getResource("internal/Finalizer.class")).isNotNull();
   }
 
+  @Test
   public void testFinalizeClassHasNoNestedClasses() throws Exception {
     // Ensure that the Finalizer class has no nested classes.
     // See https://github.com/google/guava/issues/1505
-    assertEquals(Collections.emptyList(), Arrays.asList(Finalizer.class.getDeclaredClasses()));
+    assertThat(Finalizer.class.getDeclaredClasses()).isEmpty();
+  }
+
+  static class MyServerExampleWithFrq implements Closeable {
+    private static final FinalizableReferenceQueue frq = new FinalizableReferenceQueue();
+
+    private static final Set<Reference<?>> references = Sets.newConcurrentHashSet();
+
+    private final ServerSocket serverSocket;
+
+    private MyServerExampleWithFrq() throws IOException {
+      this.serverSocket = new ServerSocket(0);
+    }
+
+    static MyServerExampleWithFrq create(AtomicBoolean finalizeReferentRan) throws IOException {
+      MyServerExampleWithFrq myServer = new MyServerExampleWithFrq();
+      ServerSocket serverSocket = myServer.serverSocket;
+      Reference<?> reference =
+          new FinalizablePhantomReference<MyServerExampleWithFrq>(myServer, frq) {
+            @Override
+            public void finalizeReferent() {
+              references.remove(this);
+              if (!serverSocket.isClosed()) {
+                finalizeReferentRan.set(true);
+                try {
+                  serverSocket.close();
+                } catch (IOException e) {
+                  throw new UncheckedIOException(e);
+                }
+              }
+            }
+          };
+      references.add(reference);
+      return myServer;
+    }
+
+    @Override
+    public void close() throws IOException {
+      serverSocket.close();
+    }
+  }
+
+  private ServerSocket makeMyServerExampleWithFrq(AtomicBoolean finalizeReferentRan)
+      throws IOException {
+    MyServerExampleWithFrq myServer = MyServerExampleWithFrq.create(finalizeReferentRan);
+    assertThat(myServer.serverSocket.isClosed()).isFalse();
+    return myServer.serverSocket;
+  }
+
+  @Test
+  public void testMyServerExampleWithFrq() throws Exception {
+    AtomicBoolean finalizeReferentRan = new AtomicBoolean(false);
+    ServerSocket serverSocket = makeMyServerExampleWithFrq(finalizeReferentRan);
+    GcFinalization.awaitDone(finalizeReferentRan::get);
+    assertThat(serverSocket.isClosed()).isTrue();
   }
 }
diff --git a/android/guava/src/com/google/common/base/FinalizableReferenceQueue.java b/android/guava/src/com/google/common/base/FinalizableReferenceQueue.java
index 7f1afbb3904d..1662342fd5fa 100644
--- a/android/guava/src/com/google/common/base/FinalizableReferenceQueue.java
+++ b/android/guava/src/com/google/common/base/FinalizableReferenceQueue.java
@@ -32,7 +32,8 @@
 
 /**
  * A reference queue with an associated background thread that dequeues references and invokes
- * {@link FinalizableReference#finalizeReferent()} on them.
+ * {@link FinalizableReference#finalizeReferent()} on them. Java 9+ users should prefer {@link
+ * java.lang.ref.Cleaner Cleaner}; see example <a href="#cleaner">below</a>.
  *
  * <p>Keep a strong reference to this object until all of the associated referents have been
  * finalized. If this object is garbage collected earlier, the backing thread will not invoke {@code
@@ -62,8 +63,9 @@
  *
  *   public static MyServer create(...) {
  *     MyServer myServer = new MyServer(...);
- *     final ServerSocket serverSocket = myServer.serverSocket;
+ *     ServerSocket serverSocket = myServer.serverSocket;
  *     Reference<?> reference = new FinalizablePhantomReference<MyServer>(myServer, frq) {
+ *       @Override
  *       public void finalizeReferent() {
  *         references.remove(this):
  *         if (!serverSocket.isClosed()) {
@@ -80,12 +82,57 @@
  *     return myServer;
  *   }
  *
- *   public void close() {
+ *   @Override
+ *   public void close() throws IOException {
  *     serverSocket.close();
  *   }
  * }
  * }</pre>
  *
+ * <p id="cleaner">Here is how you might achieve the same thing using {@link java.lang.ref.Cleaner
+ * Cleaner}, if you are using a Java version where that is available:
+ *
+ * <pre>{@code
+ * public class MyServer implements Closeable {
+ *   private static final Cleaner cleaner = Cleaner.create();
+ *   // You might also share this between several objects.
+ *
+ *   private final ServerSocket serverSocket;
+ *   private final Cleaner.Cleanable cleanable;
+ *
+ *   public MyServer(...) {
+ *     ...
+ *     this.serverSocket = new ServerSocket(...);
+ *     this.cleanable = cleaner.register(this, closeServerSocketRunnable(serverSocket));
+ *     ...
+ *   }
+ *
+ *   private static Runnable closeServerSocketRunnable(ServerSocket serverSocket) {
+ *     return () -> {
+ *       if (!serverSocket.isClosed()) {
+ *         ...log a message about how nobody called close()...
+ *         try {
+ *           serverSocket.close();
+ *         } catch (IOException e) {
+ *           ...
+ *         }
+ *       }
+ *     };
+ *   }
+ *
+ *   @Override
+ *   public void close() throws IOException {
+ *     serverSocket.close();
+ *     cleanable.clean();
+ *   }
+ * }
+ * }</pre>
+ *
+ * <p>Some care is needed when using {@code Cleaner} to ensure that the callback passed to {@code
+ * register} does not have a reference to the object (in this case, {@code MyServer}) that may be
+ * garbage-collected. That's why we are careful to make a {@code Runnable} that does not have a
+ * reference to any {@code MyServer} instance.
+ *
  * @author Bob Lee
  * @since 2.0
  */
diff --git a/guava-tests/test/com/google/common/base/FinalizableReferenceQueueTest.java b/guava-tests/test/com/google/common/base/FinalizableReferenceQueueTest.java
index 5b2dce787572..db1288c4dca9 100644
--- a/guava-tests/test/com/google/common/base/FinalizableReferenceQueueTest.java
+++ b/guava-tests/test/com/google/common/base/FinalizableReferenceQueueTest.java
@@ -16,17 +16,28 @@
 
 package com.google.common.base;
 
+import static com.google.common.truth.Truth.assertThat;
+
 import com.google.common.annotations.GwtIncompatible;
 import com.google.common.base.internal.Finalizer;
+import com.google.common.collect.Sets;
 import com.google.common.testing.GcFinalization;
+import java.io.Closeable;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.lang.ref.Reference;
 import java.lang.ref.ReferenceQueue;
 import java.lang.ref.WeakReference;
+import java.net.ServerSocket;
 import java.net.URL;
 import java.net.URLClassLoader;
-import java.util.Arrays;
-import java.util.Collections;
-import junit.framework.TestCase;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 import org.checkerframework.checker.nullness.qual.Nullable;
+import org.junit.After;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Unit test for {@link FinalizableReferenceQueue}.
@@ -38,26 +49,21 @@
 // - possibly no real concept of separate ClassLoaders?
 @AndroidIncompatible
 @GwtIncompatible
-public class FinalizableReferenceQueueTest extends TestCase {
+@RunWith(JUnit4.class)
+public class FinalizableReferenceQueueTest {
 
   private @Nullable FinalizableReferenceQueue frq;
 
-  @Override
-  protected void tearDown() throws Exception {
+  @After
+  public void tearDown() throws Exception {
     frq = null;
   }
 
-
+  @Test
   public void testFinalizeReferentCalled() {
     final MockReference reference = new MockReference(frq = new FinalizableReferenceQueue());
 
-    GcFinalization.awaitDone(
-        new GcFinalization.FinalizationPredicate() {
-          @Override
-          public boolean isDone() {
-            return reference.finalizeReferentCalled;
-          }
-        });
+    GcFinalization.awaitDone(() -> reference.finalizeReferentCalled);
   }
 
   static class MockReference extends FinalizableWeakReference<Object> {
@@ -80,7 +86,7 @@ public void finalizeReferent() {
    */
   private WeakReference<ReferenceQueue<Object>> queueReference;
 
-
+  @Test
   public void testThatFinalizerStops() {
     weaklyReferenceQueue();
     GcFinalization.awaitClear(queueReference);
@@ -109,6 +115,7 @@ public void finalizeReferent() {
         };
   }
 
+  @Test
   public void testDecoupledLoader() {
     FinalizableReferenceQueue.DecoupledLoader decoupledLoader =
         new FinalizableReferenceQueue.DecoupledLoader() {
@@ -120,10 +127,10 @@ URLClassLoader newLoader(URL base) {
 
     Class<?> finalizerCopy = decoupledLoader.loadFinalizer();
 
-    assertNotNull(finalizerCopy);
-    assertNotSame(Finalizer.class, finalizerCopy);
+    assertThat(finalizerCopy).isNotNull();
+    assertThat(finalizerCopy).isNotSameInstanceAs(Finalizer.class);
 
-    assertNotNull(FinalizableReferenceQueue.getStartFinalizer(finalizerCopy));
+    assertThat(FinalizableReferenceQueue.getStartFinalizer(finalizerCopy)).isNotNull();
   }
 
   static class DecoupledClassLoader extends URLClassLoader {
@@ -148,13 +155,69 @@ protected synchronized Class<?> loadClass(String name, boolean resolve)
     }
   }
 
+  @Test
   public void testGetFinalizerUrl() {
-    assertNotNull(getClass().getResource("internal/Finalizer.class"));
+    assertThat(getClass().getResource("internal/Finalizer.class")).isNotNull();
   }
 
+  @Test
   public void testFinalizeClassHasNoNestedClasses() throws Exception {
     // Ensure that the Finalizer class has no nested classes.
     // See https://github.com/google/guava/issues/1505
-    assertEquals(Collections.emptyList(), Arrays.asList(Finalizer.class.getDeclaredClasses()));
+    assertThat(Finalizer.class.getDeclaredClasses()).isEmpty();
+  }
+
+  static class MyServerExampleWithFrq implements Closeable {
+    private static final FinalizableReferenceQueue frq = new FinalizableReferenceQueue();
+
+    private static final Set<Reference<?>> references = Sets.newConcurrentHashSet();
+
+    private final ServerSocket serverSocket;
+
+    private MyServerExampleWithFrq() throws IOException {
+      this.serverSocket = new ServerSocket(0);
+    }
+
+    static MyServerExampleWithFrq create(AtomicBoolean finalizeReferentRan) throws IOException {
+      MyServerExampleWithFrq myServer = new MyServerExampleWithFrq();
+      ServerSocket serverSocket = myServer.serverSocket;
+      Reference<?> reference =
+          new FinalizablePhantomReference<MyServerExampleWithFrq>(myServer, frq) {
+            @Override
+            public void finalizeReferent() {
+              references.remove(this);
+              if (!serverSocket.isClosed()) {
+                finalizeReferentRan.set(true);
+                try {
+                  serverSocket.close();
+                } catch (IOException e) {
+                  throw new UncheckedIOException(e);
+                }
+              }
+            }
+          };
+      references.add(reference);
+      return myServer;
+    }
+
+    @Override
+    public void close() throws IOException {
+      serverSocket.close();
+    }
+  }
+
+  private ServerSocket makeMyServerExampleWithFrq(AtomicBoolean finalizeReferentRan)
+      throws IOException {
+    MyServerExampleWithFrq myServer = MyServerExampleWithFrq.create(finalizeReferentRan);
+    assertThat(myServer.serverSocket.isClosed()).isFalse();
+    return myServer.serverSocket;
+  }
+
+  @Test
+  public void testMyServerExampleWithFrq() throws Exception {
+    AtomicBoolean finalizeReferentRan = new AtomicBoolean(false);
+    ServerSocket serverSocket = makeMyServerExampleWithFrq(finalizeReferentRan);
+    GcFinalization.awaitDone(finalizeReferentRan::get);
+    assertThat(serverSocket.isClosed()).isTrue();
   }
 }
diff --git a/guava/src/com/google/common/base/FinalizableReferenceQueue.java b/guava/src/com/google/common/base/FinalizableReferenceQueue.java
index 30f7781e62ce..7b087837a837 100644
--- a/guava/src/com/google/common/base/FinalizableReferenceQueue.java
+++ b/guava/src/com/google/common/base/FinalizableReferenceQueue.java
@@ -32,7 +32,8 @@
 
 /**
  * A reference queue with an associated background thread that dequeues references and invokes
- * {@link FinalizableReference#finalizeReferent()} on them.
+ * {@link FinalizableReference#finalizeReferent()} on them. Java 9+ users should prefer {@link
+ * java.lang.ref.Cleaner Cleaner}; see example <a href="#cleaner">below</a>.
  *
  * <p>Keep a strong reference to this object until all of the associated referents have been
  * finalized. If this object is garbage collected earlier, the backing thread will not invoke {@code
@@ -62,8 +63,9 @@
  *
  *   public static MyServer create(...) {
  *     MyServer myServer = new MyServer(...);
- *     final ServerSocket serverSocket = myServer.serverSocket;
+ *     ServerSocket serverSocket = myServer.serverSocket;
  *     Reference<?> reference = new FinalizablePhantomReference<MyServer>(myServer, frq) {
+ *       @Override
  *       public void finalizeReferent() {
  *         references.remove(this):
  *         if (!serverSocket.isClosed()) {
@@ -80,12 +82,57 @@
  *     return myServer;
  *   }
  *
- *   public void close() {
+ *   @Override
+ *   public void close() throws IOException {
  *     serverSocket.close();
  *   }
  * }
  * }</pre>
  *
+ * <p id="cleaner">Here is how you might achieve the same thing using {@link java.lang.ref.Cleaner
+ * Cleaner}, if you are using a Java version where that is available:
+ *
+ * <pre>{@code
+ * public class MyServer implements Closeable {
+ *   private static final Cleaner cleaner = Cleaner.create();
+ *   // You might also share this between several objects.
+ *
+ *   private final ServerSocket serverSocket;
+ *   private final Cleaner.Cleanable cleanable;
+ *
+ *   public MyServer(...) {
+ *     ...
+ *     this.serverSocket = new ServerSocket(...);
+ *     this.cleanable = cleaner.register(this, closeServerSocketRunnable(serverSocket));
+ *     ...
+ *   }
+ *
+ *   private static Runnable closeServerSocketRunnable(ServerSocket serverSocket) {
+ *     return () -> {
+ *       if (!serverSocket.isClosed()) {
+ *         ...log a message about how nobody called close()...
+ *         try {
+ *           serverSocket.close();
+ *         } catch (IOException e) {
+ *           ...
+ *         }
+ *       }
+ *     };
+ *   }
+ *
+ *   @Override
+ *   public void close() throws IOException {
+ *     serverSocket.close();
+ *     cleanable.clean();
+ *   }
+ * }
+ * }</pre>
+ *
+ * <p>Some care is needed when using {@code Cleaner} to ensure that the callback passed to {@code
+ * register} does not have a reference to the object (in this case, {@code MyServer}) that may be
+ * garbage-collected. That's why we are careful to make a {@code Runnable} that does not have a
+ * reference to any {@code MyServer} instance.
+ *
  * @author Bob Lee
  * @since 2.0
  */