Skip to content

Commit ed9f40d

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 ed9f40d

File tree

2 files changed

+45
-101
lines changed

2 files changed

+45
-101
lines changed

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

+22-39
Original file line numberDiff line numberDiff line change
@@ -382,6 +382,22 @@ void unpark() {
382382

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

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

402418
/** All listeners. */
403-
private volatile @Nullable Listener listenersField;
419+
volatile @Nullable Listener listenersField;
404420

405421
/** All waiting threads. */
406-
private volatile @Nullable Waiter waitersField;
422+
volatile @Nullable Waiter waitersField;
407423

408424
/** Non-volatile write of the thread to the {@link Waiter#thread} field. */
409425
private static void putThread(Waiter waiter, Thread newValue) {
@@ -618,13 +634,13 @@ private static final class AtomicReferenceFieldUpdaterAtomicHelper extends Atomi
618634
Waiter.class, Waiter.class, "next");
619635
private static final AtomicReferenceFieldUpdater<
620636
? super AbstractFutureState<?>, @Nullable Waiter>
621-
waitersUpdater = waitersUpdaterFromWithinAbstractFutureState();
637+
waitersUpdater = newUpdater(AbstractFutureState.class, Waiter.class, "waitersField");
622638
private static final AtomicReferenceFieldUpdater<
623639
? super AbstractFutureState<?>, @Nullable Listener>
624-
listenersUpdater = listenersUpdaterFromWithinAbstractFutureState();
640+
listenersUpdater = newUpdater(AbstractFutureState.class, Listener.class, "listenersField");
625641
private static final AtomicReferenceFieldUpdater<
626642
? super AbstractFutureState<?>, @Nullable Object>
627-
valueUpdater = valueUpdaterFromWithinAbstractFutureState();
643+
valueUpdater = newUpdater(AbstractFutureState.class, Object.class, "valueField");
628644

629645
@Override
630646
void putThread(Waiter waiter, Thread newValue) {
@@ -664,39 +680,6 @@ boolean casValue(AbstractFutureState<?> future, @Nullable Object expect, Object
664680
}
665681
}
666682

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-
700683
/**
701684
* {@link AtomicHelper} based on {@code synchronized} and volatile writes.
702685
*

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

+23-62
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,22 @@ void unpark() {
388388

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

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

408424
/** All listeners. */
409-
private volatile @Nullable Listener listenersField;
425+
volatile @Nullable Listener listenersField;
410426

411427
/** All waiting threads. */
412-
private volatile @Nullable Waiter waitersField;
428+
volatile @Nullable Waiter waitersField;
413429

414430
/** Non-volatile write of the thread to the {@link Waiter#thread} field. */
415431
private static void putThread(Waiter waiter, Thread newValue) {
@@ -553,7 +569,7 @@ private static final class VarHandleAtomicHelper extends AtomicHelper {
553569
static final VarHandle valueUpdater;
554570

555571
static {
556-
MethodHandles.Lookup lookup = methodHandlesLookupFromWithinAbstractFutureState();
572+
MethodHandles.Lookup lookup = MethodHandles.lookup();
557573
try {
558574
waiterThreadUpdater = lookup.findVarHandle(Waiter.class, "thread", Thread.class);
559575
waiterNextUpdater = lookup.findVarHandle(Waiter.class, "next", Waiter.class);
@@ -610,28 +626,6 @@ private static LinkageError newLinkageError(Throwable cause) {
610626
}
611627
}
612628

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-
635629
/**
636630
* {@link AtomicHelper} based on {@link sun.misc.Unsafe}.
637631
*
@@ -734,13 +728,13 @@ private static final class AtomicReferenceFieldUpdaterAtomicHelper extends Atomi
734728
Waiter.class, Waiter.class, "next");
735729
private static final AtomicReferenceFieldUpdater<
736730
? super AbstractFutureState<?>, @Nullable Waiter>
737-
waitersUpdater = waitersUpdaterFromWithinAbstractFutureState();
731+
waitersUpdater = newUpdater(AbstractFutureState.class, Waiter.class, "waitersField");
738732
private static final AtomicReferenceFieldUpdater<
739733
? super AbstractFutureState<?>, @Nullable Listener>
740-
listenersUpdater = listenersUpdaterFromWithinAbstractFutureState();
734+
listenersUpdater = newUpdater(AbstractFutureState.class, Listener.class, "listenersField");
741735
private static final AtomicReferenceFieldUpdater<
742736
? super AbstractFutureState<?>, @Nullable Object>
743-
valueUpdater = valueUpdaterFromWithinAbstractFutureState();
737+
valueUpdater = newUpdater(AbstractFutureState.class, Object.class, "valueField");
744738

745739
@Override
746740
void putThread(Waiter waiter, Thread newValue) {
@@ -780,39 +774,6 @@ boolean casValue(AbstractFutureState<?> future, @Nullable Object expect, Object
780774
}
781775
}
782776

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-
816777
/**
817778
* {@link AtomicHelper} based on {@code synchronized} and volatile writes.
818779
*

0 commit comments

Comments
 (0)