Minor followup from cl/645331066:

- Use `@GuardedBy` where applicable. - Fix a few comments and variable names. - Resolve additional test diffs between JRE and Android flavors. RELNOTES=n/a PiperOrigin-RevId: 645490884
google · Jun 21, 2024 · faf36af · faf36af
1 parent cd11e83
commit faf36af
Show file tree

Hide file tree

Showing 11 changed files with 66 additions and 49 deletions.
diff --git a/android/guava-testlib/src/com/google/common/testing/TestLogHandler.java b/android/guava-testlib/src/com/google/common/testing/TestLogHandler.java
@@ -17,6 +17,7 @@
 package com.google.common.testing;
 
 import com.google.common.annotations.GwtCompatible;
+import com.google.errorprone.annotations.concurrent.GuardedBy;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
@@ -57,6 +58,7 @@ public class TestLogHandler extends Handler {
   private final Object lock = new Object();
 
   /** We will keep a private list of all logged records */
+  @GuardedBy("lock")
   private final List<LogRecord> list = new ArrayList<>();
 
   /** Adds the most recently logged record to our list. */

diff --git a/android/guava-tests/test/com/google/common/collect/SynchronizedMultimapTest.java b/android/guava-tests/test/com/google/common/collect/SynchronizedMultimapTest.java
@@ -133,7 +133,7 @@ public boolean containsEntry(@Nullable Object key, @Nullable Object value) {
     @Override
     public Set<V> get(@Nullable K key) {
       assertTrue(Thread.holdsLock(mutex));
-      /* TODO: verify that the Collection is also synchronized? */
+      /* TODO: verify that the Set is also synchronized? */
       return super.get(key);
     }
 
@@ -189,7 +189,7 @@ public Set<K> keySet() {
     @Override
     public Multiset<K> keys() {
       assertTrue(Thread.holdsLock(mutex));
-      /* TODO: verify that the Set is also synchronized? */
+      /* TODO: verify that the Multiset is also synchronized? */
       return super.keys();
     }
 
@@ -203,7 +203,7 @@ public Collection<V> values() {
     @Override
     public Set<Entry<K, V>> entries() {
       assertTrue(Thread.holdsLock(mutex));
-      /* TODO: verify that the Collection is also synchronized? */
+      /* TODO: verify that the Set is also synchronized? */
       return super.entries();
     }
 

diff --git a/android/guava-tests/test/com/google/common/collect/SynchronizedNavigableSetTest.java b/android/guava-tests/test/com/google/common/collect/SynchronizedNavigableSetTest.java
@@ -195,50 +195,50 @@ public List<String> order(List<String> insertionOrder) {
   }
 
   public void testDescendingSet() {
-    NavigableSet<String> map = create();
-    NavigableSet<String> descendingSet = map.descendingSet();
+    NavigableSet<String> set = create();
+    NavigableSet<String> descendingSet = set.descendingSet();
     assertTrue(descendingSet instanceof SynchronizedNavigableSet);
     assertSame(MUTEX, ((SynchronizedNavigableSet<String>) descendingSet).mutex);
   }
 
   public void testHeadSet_E() {
-    NavigableSet<String> map = create();
-    SortedSet<String> headSet = map.headSet("a");
+    NavigableSet<String> set = create();
+    SortedSet<String> headSet = set.headSet("a");
     assertTrue(headSet instanceof SynchronizedSortedSet);
     assertSame(MUTEX, ((SynchronizedSortedSet<String>) headSet).mutex);
   }
 
   public void testHeadSet_E_B() {
-    NavigableSet<String> map = create();
-    NavigableSet<String> headSet = map.headSet("a", true);
+    NavigableSet<String> set = create();
+    NavigableSet<String> headSet = set.headSet("a", true);
     assertTrue(headSet instanceof SynchronizedNavigableSet);
     assertSame(MUTEX, ((SynchronizedNavigableSet<String>) headSet).mutex);
   }
 
   public void testSubSet_E_E() {
-    NavigableSet<String> map = create();
-    SortedSet<String> subSet = map.subSet("a", "b");
+    NavigableSet<String> set = create();
+    SortedSet<String> subSet = set.subSet("a", "b");
     assertTrue(subSet instanceof SynchronizedSortedSet);
     assertSame(MUTEX, ((SynchronizedSortedSet<String>) subSet).mutex);
   }
 
   public void testSubSet_E_B_E_B() {
-    NavigableSet<String> map = create();
-    NavigableSet<String> subSet = map.subSet("a", false, "b", true);
+    NavigableSet<String> set = create();
+    NavigableSet<String> subSet = set.subSet("a", false, "b", true);
     assertTrue(subSet instanceof SynchronizedNavigableSet);
     assertSame(MUTEX, ((SynchronizedNavigableSet<String>) subSet).mutex);
   }
 
   public void testTailSet_E() {
-    NavigableSet<String> map = create();
-    SortedSet<String> tailSet = map.tailSet("a");
+    NavigableSet<String> set = create();
+    SortedSet<String> tailSet = set.tailSet("a");
     assertTrue(tailSet instanceof SynchronizedSortedSet);
     assertSame(MUTEX, ((SynchronizedSortedSet<String>) tailSet).mutex);
   }
 
   public void testTailSet_E_B() {
-    NavigableSet<String> map = create();
-    NavigableSet<String> tailSet = map.tailSet("a", true);
+    NavigableSet<String> set = create();
+    NavigableSet<String> tailSet = set.tailSet("a", true);
     assertTrue(tailSet instanceof SynchronizedNavigableSet);
     assertSame(MUTEX, ((SynchronizedNavigableSet<String>) tailSet).mutex);
   }

diff --git a/android/guava-tests/test/com/google/common/collect/SynchronizedSetTest.java b/android/guava-tests/test/com/google/common/collect/SynchronizedSetTest.java
@@ -40,9 +40,6 @@ public class SynchronizedSetTest extends TestCase {
 
   public static final Object MUTEX = new Integer(1); // something Serializable
 
-  // TODO(cpovirk): Resolve difference between branches in their choice of mutex:
-  // - The mainline uses `null` (even since the change in cl/99720576 was integrated).
-  // - The backport continued to use MUTEX.
   public static Test suite() {
     return SetTestSuiteBuilder.using(
             new TestStringSetGenerator() {
@@ -132,6 +129,25 @@ public boolean isEmpty() {
       return super.isEmpty();
     }
 
+    /*
+     * We don't assert that the lock is held during calls to iterator(), stream(), and spliterator:
+     * `Synchronized` doesn't guarantee that it will hold the mutex for those calls because callers
+     * are responsible for taking the mutex themselves:
+     * https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/Collections.html#synchronizedCollection(java.util.Collection)
+     *
+     * Similarly, we avoid having those methods *implemented* in terms of *other* TestSet methods
+     * that will perform holdsLock assertions:
+     *
+     * - For iterator(), we can accomplish that by not overriding iterator() at all. That way, we
+     *   inherit an implementation that forwards to the delegate collection, which performs no
+     *   holdsLock assertions.
+     *
+     * - For stream() and spliterator(), we have to forward to the delegate ourselves because
+     *   ForwardingSet does not forward `default` methods, as discussed in its Javadoc.
+     */
+
+    // Currently, we don't include stream() and spliterator() for our classes in the Android flavor.
+
     @Override
     public boolean remove(@Nullable Object o) {
       assertTrue(Thread.holdsLock(mutex));

diff --git a/android/guava/src/com/google/common/util/concurrent/LazyLogger.java b/android/guava/src/com/google/common/util/concurrent/LazyLogger.java
@@ -34,7 +34,7 @@ final class LazyLogger {
   Logger get() {
     /*
      * We use double-checked locking. We could the try racy single-check idiom, but that would
-     * depend on Logger not contain mutable state.
+     * depend on Logger to not contain mutable state.
      *
      * We could use Suppliers.memoizingSupplier here, but I micro-optimized to this implementation
      * to avoid the extra class for the lambda (and maybe more for memoizingSupplier itself) and the

diff --git a/guava-testlib/src/com/google/common/testing/TestLogHandler.java b/guava-testlib/src/com/google/common/testing/TestLogHandler.java
@@ -17,6 +17,7 @@
 package com.google.common.testing;
 
 import com.google.common.annotations.GwtCompatible;
+import com.google.errorprone.annotations.concurrent.GuardedBy;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
@@ -57,6 +58,7 @@ public class TestLogHandler extends Handler {
   private final Object lock = new Object();
 
   /** We will keep a private list of all logged records */
+  @GuardedBy("lock")
   private final List<LogRecord> list = new ArrayList<>();
 
   /** Adds the most recently logged record to our list. */

diff --git a/guava-tests/test/com/google/common/collect/SynchronizedMultimapTest.java b/guava-tests/test/com/google/common/collect/SynchronizedMultimapTest.java
@@ -133,7 +133,7 @@ public boolean containsEntry(@Nullable Object key, @Nullable Object value) {
     @Override
     public Set<V> get(@Nullable K key) {
       assertTrue(Thread.holdsLock(mutex));
-      /* TODO: verify that the Collection is also synchronized? */
+      /* TODO: verify that the Set is also synchronized? */
       return super.get(key);
     }
 
@@ -189,7 +189,7 @@ public Set<K> keySet() {
     @Override
     public Multiset<K> keys() {
       assertTrue(Thread.holdsLock(mutex));
-      /* TODO: verify that the Set is also synchronized? */
+      /* TODO: verify that the Multiset is also synchronized? */
       return super.keys();
     }
 
@@ -203,7 +203,7 @@ public Collection<V> values() {
     @Override
     public Set<Entry<K, V>> entries() {
       assertTrue(Thread.holdsLock(mutex));
-      /* TODO: verify that the Collection is also synchronized? */
+      /* TODO: verify that the Set is also synchronized? */
       return super.entries();
     }
 

diff --git a/guava-tests/test/com/google/common/collect/SynchronizedNavigableSetTest.java b/guava-tests/test/com/google/common/collect/SynchronizedNavigableSetTest.java
@@ -40,13 +40,13 @@
  * @author Louis Wasserman
  */
 public class SynchronizedNavigableSetTest extends TestCase {
+  private static final Object MUTEX = new Integer(1); // something Serializable
 
   @SuppressWarnings("unchecked")
   protected <E> NavigableSet<E> create() {
     TestSet<E> inner =
-        new TestSet<>(new TreeSet<E>((Comparator<E>) Ordering.natural().nullsFirst()), null);
-    NavigableSet<E> outer = Synchronized.navigableSet(inner, null);
-    inner.mutex = outer;
+        new TestSet<>(new TreeSet<E>((Comparator<E>) Ordering.natural().nullsFirst()), MUTEX);
+    NavigableSet<E> outer = Synchronized.navigableSet(inner, MUTEX);
     return outer;
   }
 
@@ -173,9 +173,8 @@ public static TestSuite suite() {
                   protected NavigableSet<String> create(String[] elements) {
                     NavigableSet<String> innermost = new SafeTreeSet<>();
                     Collections.addAll(innermost, elements);
-                    TestSet<String> inner = new TestSet<>(innermost, null);
-                    NavigableSet<String> outer = Synchronized.navigableSet(inner, null);
-                    inner.mutex = outer;
+                    TestSet<String> inner = new TestSet<>(innermost, MUTEX);
+                    NavigableSet<String> outer = Synchronized.navigableSet(inner, MUTEX);
                     return outer;
                   }
 
@@ -199,48 +198,48 @@ public void testDescendingSet() {
     NavigableSet<String> set = create();
     NavigableSet<String> descendingSet = set.descendingSet();
     assertTrue(descendingSet instanceof SynchronizedNavigableSet);
-    assertSame(set, ((SynchronizedNavigableSet<String>) descendingSet).mutex);
+    assertSame(MUTEX, ((SynchronizedNavigableSet<String>) descendingSet).mutex);
   }
 
   public void testHeadSet_E() {
     NavigableSet<String> set = create();
     SortedSet<String> headSet = set.headSet("a");
     assertTrue(headSet instanceof SynchronizedSortedSet);
-    assertSame(set, ((SynchronizedSortedSet<String>) headSet).mutex);
+    assertSame(MUTEX, ((SynchronizedSortedSet<String>) headSet).mutex);
   }
 
   public void testHeadSet_E_B() {
     NavigableSet<String> set = create();
     NavigableSet<String> headSet = set.headSet("a", true);
     assertTrue(headSet instanceof SynchronizedNavigableSet);
-    assertSame(set, ((SynchronizedNavigableSet<String>) headSet).mutex);
+    assertSame(MUTEX, ((SynchronizedNavigableSet<String>) headSet).mutex);
   }
 
   public void testSubSet_E_E() {
     NavigableSet<String> set = create();
     SortedSet<String> subSet = set.subSet("a", "b");
     assertTrue(subSet instanceof SynchronizedSortedSet);
-    assertSame(set, ((SynchronizedSortedSet<String>) subSet).mutex);
+    assertSame(MUTEX, ((SynchronizedSortedSet<String>) subSet).mutex);
   }
 
   public void testSubSet_E_B_E_B() {
     NavigableSet<String> set = create();
     NavigableSet<String> subSet = set.subSet("a", false, "b", true);
     assertTrue(subSet instanceof SynchronizedNavigableSet);
-    assertSame(set, ((SynchronizedNavigableSet<String>) subSet).mutex);
+    assertSame(MUTEX, ((SynchronizedNavigableSet<String>) subSet).mutex);
   }
 
   public void testTailSet_E() {
     NavigableSet<String> set = create();
     SortedSet<String> tailSet = set.tailSet("a");
     assertTrue(tailSet instanceof SynchronizedSortedSet);
-    assertSame(set, ((SynchronizedSortedSet<String>) tailSet).mutex);
+    assertSame(MUTEX, ((SynchronizedSortedSet<String>) tailSet).mutex);
   }
 
   public void testTailSet_E_B() {
     NavigableSet<String> set = create();
     NavigableSet<String> tailSet = set.tailSet("a", true);
     assertTrue(tailSet instanceof SynchronizedNavigableSet);
-    assertSame(set, ((SynchronizedNavigableSet<String>) tailSet).mutex);
+    assertSame(MUTEX, ((SynchronizedNavigableSet<String>) tailSet).mutex);
   }
 }
diff --git a/guava-tests/test/com/google/common/collect/SynchronizedQueueTest.java b/guava-tests/test/com/google/common/collect/SynchronizedQueueTest.java
@@ -32,15 +32,14 @@ public class SynchronizedQueueTest extends TestCase {
 
   protected Queue<String> create() {
     TestQueue<String> inner = new TestQueue<>();
-    Queue<String> outer = Synchronized.queue(inner, null);
-    inner.mutex = outer;
+    Queue<String> outer = Synchronized.queue(inner, inner.mutex);
     outer.add("foo"); // necessary because we try to remove elements later on
     return outer;
   }
 
   private static final class TestQueue<E> implements Queue<E> {
     private final Queue<E> delegate = Lists.newLinkedList();
-    public Object mutex;
+    public final Object mutex = new Integer(1); // something Serializable
 
     @Override
     public boolean offer(E o) {

diff --git a/guava-tests/test/com/google/common/collect/SynchronizedSetTest.java b/guava-tests/test/com/google/common/collect/SynchronizedSetTest.java
@@ -16,6 +16,8 @@
 
 package com.google.common.collect;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import com.google.common.collect.testing.SetTestSuiteBuilder;
 import com.google.common.collect.testing.TestStringSetGenerator;
 import com.google.common.collect.testing.features.CollectionFeature;
@@ -40,17 +42,13 @@ public class SynchronizedSetTest extends TestCase {
 
   public static final Object MUTEX = new Integer(1); // something Serializable
 
-  // TODO(cpovirk): Resolve difference between branches in their choice of mutex:
-  // - The mainline uses `null` (even since the change in cl/99720576 was integrated).
-  // - The backport continued to use MUTEX.
   public static Test suite() {
     return SetTestSuiteBuilder.using(
             new TestStringSetGenerator() {
               @Override
               protected Set<String> create(String[] elements) {
-                TestSet<String> inner = new TestSet<>(new HashSet<String>(), null);
-                Set<String> outer = Synchronized.set(inner, null);
-                inner.mutex = outer;
+                TestSet<String> inner = new TestSet<>(new HashSet<String>(), MUTEX);
+                Set<String> outer = Synchronized.set(inner, inner.mutex);
                 Collections.addAll(outer, elements);
                 return outer;
               }
@@ -66,9 +64,10 @@ protected Set<String> create(String[] elements) {
 
   static class TestSet<E> extends ForwardingSet<E> implements Serializable {
     final Set<E> delegate;
-    public Object mutex;
+    public final Object mutex;
 
-    public TestSet(Set<E> delegate, @Nullable Object mutex) {
+    public TestSet(Set<E> delegate, Object mutex) {
+      checkNotNull(mutex);
       this.delegate = delegate;
       this.mutex = mutex;
     }

diff --git a/guava/src/com/google/common/util/concurrent/LazyLogger.java b/guava/src/com/google/common/util/concurrent/LazyLogger.java
@@ -34,7 +34,7 @@ final class LazyLogger {
   Logger get() {
     /*
      * We use double-checked locking. We could the try racy single-check idiom, but that would
-     * depend on Logger not contain mutable state.
+     * depend on Logger to not contain mutable state.
      *
      * We could use Suppliers.memoizingSupplier here, but I micro-optimized to this implementation
      * to avoid the extra class for the lambda (and maybe more for memoizingSupplier itself) and the