From faf36af34739ddf747eb896ee448688e17e3882b Mon Sep 17 00:00:00 2001 From: cpovirk Date: Fri, 21 Jun 2024 13:49:43 -0700 Subject: [PATCH] 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/common/testing/TestLogHandler.java | 2 ++ .../collect/SynchronizedMultimapTest.java | 6 ++-- .../collect/SynchronizedNavigableSetTest.java | 28 +++++++++---------- .../common/collect/SynchronizedSetTest.java | 22 +++++++++++++-- .../common/util/concurrent/LazyLogger.java | 2 +- .../google/common/testing/TestLogHandler.java | 2 ++ .../collect/SynchronizedMultimapTest.java | 6 ++-- .../collect/SynchronizedNavigableSetTest.java | 25 ++++++++--------- .../common/collect/SynchronizedQueueTest.java | 5 ++-- .../common/collect/SynchronizedSetTest.java | 15 +++++----- .../common/util/concurrent/LazyLogger.java | 2 +- 11 files changed, 66 insertions(+), 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 index a08ed550c231..aba2131935bb 100644 --- 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 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 index 6475dad8f983..82aa4da43e78 100644 --- 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 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 keySet() { @Override public Multiset 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 values() { @Override public Set> 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 index 572bea44e33f..017c7e2bebc3 100644 --- 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 order(List insertionOrder) { } public void testDescendingSet() { - NavigableSet map = create(); - NavigableSet descendingSet = map.descendingSet(); + NavigableSet set = create(); + NavigableSet descendingSet = set.descendingSet(); assertTrue(descendingSet instanceof SynchronizedNavigableSet); assertSame(MUTEX, ((SynchronizedNavigableSet) descendingSet).mutex); } public void testHeadSet_E() { - NavigableSet map = create(); - SortedSet headSet = map.headSet("a"); + NavigableSet set = create(); + SortedSet headSet = set.headSet("a"); assertTrue(headSet instanceof SynchronizedSortedSet); assertSame(MUTEX, ((SynchronizedSortedSet) headSet).mutex); } public void testHeadSet_E_B() { - NavigableSet map = create(); - NavigableSet headSet = map.headSet("a", true); + NavigableSet set = create(); + NavigableSet headSet = set.headSet("a", true); assertTrue(headSet instanceof SynchronizedNavigableSet); assertSame(MUTEX, ((SynchronizedNavigableSet) headSet).mutex); } public void testSubSet_E_E() { - NavigableSet map = create(); - SortedSet subSet = map.subSet("a", "b"); + NavigableSet set = create(); + SortedSet subSet = set.subSet("a", "b"); assertTrue(subSet instanceof SynchronizedSortedSet); assertSame(MUTEX, ((SynchronizedSortedSet) subSet).mutex); } public void testSubSet_E_B_E_B() { - NavigableSet map = create(); - NavigableSet subSet = map.subSet("a", false, "b", true); + NavigableSet set = create(); + NavigableSet subSet = set.subSet("a", false, "b", true); assertTrue(subSet instanceof SynchronizedNavigableSet); assertSame(MUTEX, ((SynchronizedNavigableSet) subSet).mutex); } public void testTailSet_E() { - NavigableSet map = create(); - SortedSet tailSet = map.tailSet("a"); + NavigableSet set = create(); + SortedSet tailSet = set.tailSet("a"); assertTrue(tailSet instanceof SynchronizedSortedSet); assertSame(MUTEX, ((SynchronizedSortedSet) tailSet).mutex); } public void testTailSet_E_B() { - NavigableSet map = create(); - NavigableSet tailSet = map.tailSet("a", true); + NavigableSet set = create(); + NavigableSet tailSet = set.tailSet("a", true); assertTrue(tailSet instanceof SynchronizedNavigableSet); assertSame(MUTEX, ((SynchronizedNavigableSet) 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 index 54ed07152f5e..a17e5a8163ca 100644 --- 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 index ef1b56957d52..61fefe804a64 100644 --- 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 index a08ed550c231..aba2131935bb 100644 --- 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 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 index 6475dad8f983..82aa4da43e78 100644 --- 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 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 keySet() { @Override public Multiset 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 values() { @Override public Set> 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 index 93969556a22b..017c7e2bebc3 100644 --- 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 NavigableSet create() { TestSet inner = - new TestSet<>(new TreeSet((Comparator) Ordering.natural().nullsFirst()), null); - NavigableSet outer = Synchronized.navigableSet(inner, null); - inner.mutex = outer; + new TestSet<>(new TreeSet((Comparator) Ordering.natural().nullsFirst()), MUTEX); + NavigableSet outer = Synchronized.navigableSet(inner, MUTEX); return outer; } @@ -173,9 +173,8 @@ public static TestSuite suite() { protected NavigableSet create(String[] elements) { NavigableSet innermost = new SafeTreeSet<>(); Collections.addAll(innermost, elements); - TestSet inner = new TestSet<>(innermost, null); - NavigableSet outer = Synchronized.navigableSet(inner, null); - inner.mutex = outer; + TestSet inner = new TestSet<>(innermost, MUTEX); + NavigableSet outer = Synchronized.navigableSet(inner, MUTEX); return outer; } @@ -199,48 +198,48 @@ public void testDescendingSet() { NavigableSet set = create(); NavigableSet descendingSet = set.descendingSet(); assertTrue(descendingSet instanceof SynchronizedNavigableSet); - assertSame(set, ((SynchronizedNavigableSet) descendingSet).mutex); + assertSame(MUTEX, ((SynchronizedNavigableSet) descendingSet).mutex); } public void testHeadSet_E() { NavigableSet set = create(); SortedSet headSet = set.headSet("a"); assertTrue(headSet instanceof SynchronizedSortedSet); - assertSame(set, ((SynchronizedSortedSet) headSet).mutex); + assertSame(MUTEX, ((SynchronizedSortedSet) headSet).mutex); } public void testHeadSet_E_B() { NavigableSet set = create(); NavigableSet headSet = set.headSet("a", true); assertTrue(headSet instanceof SynchronizedNavigableSet); - assertSame(set, ((SynchronizedNavigableSet) headSet).mutex); + assertSame(MUTEX, ((SynchronizedNavigableSet) headSet).mutex); } public void testSubSet_E_E() { NavigableSet set = create(); SortedSet subSet = set.subSet("a", "b"); assertTrue(subSet instanceof SynchronizedSortedSet); - assertSame(set, ((SynchronizedSortedSet) subSet).mutex); + assertSame(MUTEX, ((SynchronizedSortedSet) subSet).mutex); } public void testSubSet_E_B_E_B() { NavigableSet set = create(); NavigableSet subSet = set.subSet("a", false, "b", true); assertTrue(subSet instanceof SynchronizedNavigableSet); - assertSame(set, ((SynchronizedNavigableSet) subSet).mutex); + assertSame(MUTEX, ((SynchronizedNavigableSet) subSet).mutex); } public void testTailSet_E() { NavigableSet set = create(); SortedSet tailSet = set.tailSet("a"); assertTrue(tailSet instanceof SynchronizedSortedSet); - assertSame(set, ((SynchronizedSortedSet) tailSet).mutex); + assertSame(MUTEX, ((SynchronizedSortedSet) tailSet).mutex); } public void testTailSet_E_B() { NavigableSet set = create(); NavigableSet tailSet = set.tailSet("a", true); assertTrue(tailSet instanceof SynchronizedNavigableSet); - assertSame(set, ((SynchronizedNavigableSet) tailSet).mutex); + assertSame(MUTEX, ((SynchronizedNavigableSet) tailSet).mutex); } } diff --git a/guava-tests/test/com/google/common/collect/SynchronizedQueueTest.java b/guava-tests/test/com/google/common/collect/SynchronizedQueueTest.java index e29966367a6f..b677c281b94d 100644 --- 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 create() { TestQueue inner = new TestQueue<>(); - Queue outer = Synchronized.queue(inner, null); - inner.mutex = outer; + Queue 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 implements Queue { private final Queue 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 index 1c42b92a7c08..7c31442931a5 100644 --- 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 create(String[] elements) { - TestSet inner = new TestSet<>(new HashSet(), null); - Set outer = Synchronized.set(inner, null); - inner.mutex = outer; + TestSet inner = new TestSet<>(new HashSet(), MUTEX); + Set outer = Synchronized.set(inner, inner.mutex); Collections.addAll(outer, elements); return outer; } @@ -66,9 +64,10 @@ protected Set create(String[] elements) { static class TestSet extends ForwardingSet implements Serializable { final Set delegate; - public Object mutex; + public final Object mutex; - public TestSet(Set delegate, @Nullable Object mutex) { + public TestSet(Set 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 index ef1b56957d52..61fefe804a64 100644 --- 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