Skip to content

Commit 0ec5f28

Browse files
cpovirkGoogle Java Core Libraries
authored and
Google Java Core Libraries
committed
Make AbstractFutureState fields package-private.
By keeping them `private`, we've mostly been making our lives more complicated, as discussed in cl/741607075. Currently, that would complicate using `VarHandle` in `guava-android`, and that would in turn complicate [our migration off `Unsafe`](#7742). (But then making them _package-private_ complicated things too, as seen in cl/742311780....) This CL continues the trend of work that is necessary [only because of Java 8 compatibility](#6614 (comment)). RELNOTES=n/a PiperOrigin-RevId: 741607263
1 parent 266ce00 commit 0ec5f28

File tree

2 files changed

+47
-101
lines changed

2 files changed

+47
-101
lines changed

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

+23-39
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import com.google.common.annotations.GwtCompatible;
2727
import com.google.common.util.concurrent.AbstractFuture.Listener;
2828
import com.google.common.util.concurrent.internal.InternalFutureFailureAccess;
29+
import com.google.errorprone.annotations.ThreadSafe;
2930
import com.google.j2objc.annotations.ReflectionSupport;
3031
import com.google.j2objc.annotations.RetainedLocalRef;
3132
import java.lang.reflect.Field;
@@ -382,6 +383,22 @@ void unpark() {
382383

383384
// TODO(lukes): Investigate using a @Contended annotation on these fields once one is available.
384385

386+
/*
387+
* The following fields are package-private, even though we intend never to use them outside this
388+
* file. If they were instead private, then we wouldn't be able to access them reflectively from
389+
* within VarHandleAtomicHelper.
390+
*
391+
* Package-private "shouldn't" be necessary: VarHandleAtomicHelper and AbstractFutureState
392+
* "should" be nestmates, so a call to MethodHandles.lookup inside VarHandleAtomicHelper "should"
393+
* have access to AbstractFutureState's private fields. However, our open-source build uses
394+
* `-source 8 -target 8`, so the class files from that build can't express nestmates. Thus, when
395+
* those class files are used from Java 9 or higher (i.e., high enough to trigger the VarHandle
396+
* code path), such a lookup would fail with an IllegalAccessException.
397+
*
398+
* This same problem is one of the reasons for us to likewise keep the fields in Waiter as
399+
* package-private instead of private.
400+
*/
401+
385402
/**
386403
* This field encodes the current state of the future.
387404
*
@@ -397,13 +414,13 @@ void unpark() {
397414
* argument.
398415
* </ul>
399416
*/
400-
private volatile @Nullable Object valueField;
417+
@ThreadSafe.Suppress volatile @Nullable Object valueField;
401418

402419
/** All listeners. */
403-
private volatile @Nullable Listener listenersField;
420+
@ThreadSafe.Suppress volatile @Nullable Listener listenersField;
404421

405422
/** All waiting threads. */
406-
private volatile @Nullable Waiter waitersField;
423+
@ThreadSafe.Suppress volatile @Nullable Waiter waitersField;
407424

408425
/** Non-volatile write of the thread to the {@link Waiter#thread} field. */
409426
private static void putThread(Waiter waiter, Thread newValue) {
@@ -618,13 +635,13 @@ private static final class AtomicReferenceFieldUpdaterAtomicHelper extends Atomi
618635
Waiter.class, Waiter.class, "next");
619636
private static final AtomicReferenceFieldUpdater<
620637
? super AbstractFutureState<?>, @Nullable Waiter>
621-
waitersUpdater = waitersUpdaterFromWithinAbstractFutureState();
638+
waitersUpdater = newUpdater(AbstractFutureState.class, Waiter.class, "waitersField");
622639
private static final AtomicReferenceFieldUpdater<
623640
? super AbstractFutureState<?>, @Nullable Listener>
624-
listenersUpdater = listenersUpdaterFromWithinAbstractFutureState();
641+
listenersUpdater = newUpdater(AbstractFutureState.class, Listener.class, "listenersField");
625642
private static final AtomicReferenceFieldUpdater<
626643
? super AbstractFutureState<?>, @Nullable Object>
627-
valueUpdater = valueUpdaterFromWithinAbstractFutureState();
644+
valueUpdater = newUpdater(AbstractFutureState.class, Object.class, "valueField");
628645

629646
@Override
630647
void putThread(Waiter waiter, Thread newValue) {
@@ -664,39 +681,6 @@ boolean casValue(AbstractFutureState<?> future, @Nullable Object expect, Object
664681
}
665682
}
666683

667-
/**
668-
* Returns an {@link AtomicReferenceFieldUpdater} for {@link #waiters}.
669-
*
670-
* <p>The creation of the updater has to happen directly inside {@link AbstractFutureState}, as
671-
* discussed in {@link #methodHandlesLookupFromWithinAbstractFutureState}.
672-
*/
673-
private static AtomicReferenceFieldUpdater<? super AbstractFutureState<?>, @Nullable Waiter>
674-
waitersUpdaterFromWithinAbstractFutureState() {
675-
return newUpdater(AbstractFutureState.class, Waiter.class, "waitersField");
676-
}
677-
678-
/**
679-
* Returns an {@link AtomicReferenceFieldUpdater} for {@link #listeners}.
680-
*
681-
* <p>The creation of the updater has to happen directly inside {@link AbstractFutureState}, as
682-
* discussed in {@link #methodHandlesLookupFromWithinAbstractFutureState}.
683-
*/
684-
private static AtomicReferenceFieldUpdater<? super AbstractFutureState<?>, @Nullable Listener>
685-
listenersUpdaterFromWithinAbstractFutureState() {
686-
return newUpdater(AbstractFutureState.class, Listener.class, "listenersField");
687-
}
688-
689-
/**
690-
* Returns an {@link AtomicReferenceFieldUpdater} for {@link #value}.
691-
*
692-
* <p>The creation of the updater has to happen directly inside {@link AbstractFutureState}, as
693-
* discussed in {@link #methodHandlesLookupFromWithinAbstractFutureState}.
694-
*/
695-
private static AtomicReferenceFieldUpdater<? super AbstractFutureState<?>, @Nullable Object>
696-
valueUpdaterFromWithinAbstractFutureState() {
697-
return newUpdater(AbstractFutureState.class, Object.class, "valueField");
698-
}
699-
700684
/**
701685
* {@link AtomicHelper} based on {@code synchronized} and volatile writes.
702686
*

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

+24-62
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import com.google.common.annotations.GwtCompatible;
2727
import com.google.common.util.concurrent.AbstractFuture.Listener;
2828
import com.google.common.util.concurrent.internal.InternalFutureFailureAccess;
29+
import com.google.errorprone.annotations.ThreadSafe;
2930
import com.google.j2objc.annotations.J2ObjCIncompatible;
3031
import com.google.j2objc.annotations.ReflectionSupport;
3132
import com.google.j2objc.annotations.RetainedLocalRef;
@@ -388,6 +389,22 @@ void unpark() {
388389

389390
// TODO(lukes): Investigate using a @Contended annotation on these fields once one is available.
390391

392+
/*
393+
* The following fields are package-private, even though we intend never to use them outside this
394+
* file. If they were instead private, then we wouldn't be able to access them reflectively from
395+
* within VarHandleAtomicHelper.
396+
*
397+
* Package-private "shouldn't" be necessary: VarHandleAtomicHelper and AbstractFutureState
398+
* "should" be nestmates, so a call to MethodHandles.lookup inside VarHandleAtomicHelper "should"
399+
* have access to AbstractFutureState's private fields. However, our open-source build uses
400+
* `-source 8 -target 8`, so the class files from that build can't express nestmates. Thus, when
401+
* those class files are used from Java 9 or higher (i.e., high enough to trigger the VarHandle
402+
* code path), such a lookup would fail with an IllegalAccessException.
403+
*
404+
* This same problem is one of the reasons for us to likewise keep the fields in Waiter as
405+
* package-private instead of private.
406+
*/
407+
391408
/**
392409
* This field encodes the current state of the future.
393410
*
@@ -403,13 +420,13 @@ void unpark() {
403420
* argument.
404421
* </ul>
405422
*/
406-
private volatile @Nullable Object valueField;
423+
@ThreadSafe.Suppress volatile @Nullable Object valueField;
407424

408425
/** All listeners. */
409-
private volatile @Nullable Listener listenersField;
426+
@ThreadSafe.Suppress volatile @Nullable Listener listenersField;
410427

411428
/** All waiting threads. */
412-
private volatile @Nullable Waiter waitersField;
429+
@ThreadSafe.Suppress volatile @Nullable Waiter waitersField;
413430

414431
/** Non-volatile write of the thread to the {@link Waiter#thread} field. */
415432
private static void putThread(Waiter waiter, Thread newValue) {
@@ -553,7 +570,7 @@ private static final class VarHandleAtomicHelper extends AtomicHelper {
553570
static final VarHandle valueUpdater;
554571

555572
static {
556-
MethodHandles.Lookup lookup = methodHandlesLookupFromWithinAbstractFutureState();
573+
MethodHandles.Lookup lookup = MethodHandles.lookup();
557574
try {
558575
waiterThreadUpdater = lookup.findVarHandle(Waiter.class, "thread", Thread.class);
559576
waiterNextUpdater = lookup.findVarHandle(Waiter.class, "next", Waiter.class);
@@ -610,28 +627,6 @@ private static LinkageError newLinkageError(Throwable cause) {
610627
}
611628
}
612629

613-
/**
614-
* Returns the result of calling {@link MethodHandles#lookup} from inside {@link
615-
* AbstractFutureState}. By virtue of being created there, it has access to the private fields of
616-
* {@link AbstractFutureState}, so that access is available to anyone who calls this
617-
* method—specifically, to {@link VarHandleAtomicHelper}.
618-
*
619-
* <p>This "shouldn't" be necessary: {@link VarHandleAtomicHelper} and {@link AbstractFutureState}
620-
* "should" be nestmates, so a call to {@link MethodHandles#lookup} inside {@link
621-
* VarHandleAtomicHelper} "should" have access to each other's private fields. However, our
622-
* open-source build uses {@code -source 8 -target 8}, so the class files from that build can't
623-
* express nestmates. Thus, when those class files are used from Java 9 or higher (i.e., high
624-
* enough to trigger the {@link VarHandle} code path), such a lookup would fail with an {@link
625-
* IllegalAccessException}.
626-
*
627-
* <p>Note that we do not have a similar problem with the fields in {@link Waiter} because those
628-
* fields are not private. (We could solve the problem with {@link AbstractFutureState} fields in
629-
* the same way if we wanted.)
630-
*/
631-
private static MethodHandles.Lookup methodHandlesLookupFromWithinAbstractFutureState() {
632-
return MethodHandles.lookup();
633-
}
634-
635630
/**
636631
* {@link AtomicHelper} based on {@link sun.misc.Unsafe}.
637632
*
@@ -734,13 +729,13 @@ private static final class AtomicReferenceFieldUpdaterAtomicHelper extends Atomi
734729
Waiter.class, Waiter.class, "next");
735730
private static final AtomicReferenceFieldUpdater<
736731
? super AbstractFutureState<?>, @Nullable Waiter>
737-
waitersUpdater = waitersUpdaterFromWithinAbstractFutureState();
732+
waitersUpdater = newUpdater(AbstractFutureState.class, Waiter.class, "waitersField");
738733
private static final AtomicReferenceFieldUpdater<
739734
? super AbstractFutureState<?>, @Nullable Listener>
740-
listenersUpdater = listenersUpdaterFromWithinAbstractFutureState();
735+
listenersUpdater = newUpdater(AbstractFutureState.class, Listener.class, "listenersField");
741736
private static final AtomicReferenceFieldUpdater<
742737
? super AbstractFutureState<?>, @Nullable Object>
743-
valueUpdater = valueUpdaterFromWithinAbstractFutureState();
738+
valueUpdater = newUpdater(AbstractFutureState.class, Object.class, "valueField");
744739

745740
@Override
746741
void putThread(Waiter waiter, Thread newValue) {
@@ -780,39 +775,6 @@ boolean casValue(AbstractFutureState<?> future, @Nullable Object expect, Object
780775
}
781776
}
782777

783-
/**
784-
* Returns an {@link AtomicReferenceFieldUpdater} for {@link #waiters}.
785-
*
786-
* <p>The creation of the updater has to happen directly inside {@link AbstractFutureState}, as
787-
* discussed in {@link #methodHandlesLookupFromWithinAbstractFutureState}.
788-
*/
789-
private static AtomicReferenceFieldUpdater<? super AbstractFutureState<?>, @Nullable Waiter>
790-
waitersUpdaterFromWithinAbstractFutureState() {
791-
return newUpdater(AbstractFutureState.class, Waiter.class, "waitersField");
792-
}
793-
794-
/**
795-
* Returns an {@link AtomicReferenceFieldUpdater} for {@link #listeners}.
796-
*
797-
* <p>The creation of the updater has to happen directly inside {@link AbstractFutureState}, as
798-
* discussed in {@link #methodHandlesLookupFromWithinAbstractFutureState}.
799-
*/
800-
private static AtomicReferenceFieldUpdater<? super AbstractFutureState<?>, @Nullable Listener>
801-
listenersUpdaterFromWithinAbstractFutureState() {
802-
return newUpdater(AbstractFutureState.class, Listener.class, "listenersField");
803-
}
804-
805-
/**
806-
* Returns an {@link AtomicReferenceFieldUpdater} for {@link #value}.
807-
*
808-
* <p>The creation of the updater has to happen directly inside {@link AbstractFutureState}, as
809-
* discussed in {@link #methodHandlesLookupFromWithinAbstractFutureState}.
810-
*/
811-
private static AtomicReferenceFieldUpdater<? super AbstractFutureState<?>, @Nullable Object>
812-
valueUpdaterFromWithinAbstractFutureState() {
813-
return newUpdater(AbstractFutureState.class, Object.class, "valueField");
814-
}
815-
816778
/**
817779
* {@link AtomicHelper} based on {@code synchronized} and volatile writes.
818780
*

0 commit comments

Comments
 (0)