RFR: 8344197: SM cleanup in java.util.concurrent

[Alan Bateman]

On Thu, 14 Nov 2024 20:45:49 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

> Refactored to remove use of doPrivileged() and use of SecurityManager.
> The DefaultForkJoinWorkerThreadFactory no longer uses the SM to target a common thread pool.
> 
> A careful review is requested.

I did a pass over this. Most of it is okay but there are a couple of issues.

There are significant changes to ForkJoinPool under review and due to integrate any day now. I'll defer to @DougLea and @viktorklang-ora but it might be better to drop the changes to FJP from this PR and follow-up after the FJP upgrade is integrated.

src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java line 2104:

> 2102:             lockField.set(this, new Object());
> 2103:         } catch (IllegalAccessException | NoSuchFieldException e) {
> 2104:                 throw new RuntimeException(e);

Did you mean to change this from Error to RuntimeException? It shouldn't happen of course, but probably best not to change this.

src/java.base/share/classes/java/util/concurrent/Executors.java line 556:

> 554:      * A callable that runs under established access control settings.
> 555:      */
> 556:     private static final class PrivilegedCallable<T> implements Callable<T> {

I can't help thinking some larger cleanup or renaming will be needed here as PrivilegedXXX is misleading now.

src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java line 1108:

> 1106:         implements ForkJoinWorkerThreadFactory {
> 1107:         public final ForkJoinWorkerThread newThread(ForkJoinPool pool) {
> 1108:             return new ForkJoinWorkerThread(null, pool, true, false);

This doesn't look right. For the common pool it should return InnocuousForkJoinWorkerThread(pool).

src/java.base/share/classes/java/util/concurrent/ForkJoinWorkerThread.java line 244:

> 242:         @Override // paranoically
> 243:         public void setContextClassLoader(ClassLoader cl) {
> 244:             // No-op, do not change the context class loader

I don't think this we should change this in this PR. Instead, I think we need to decide what to specify.

src/java.base/share/classes/java/util/concurrent/StructuredTaskScope.java line 700:

> 698: 
> 699:     private void interruptAll() {
> 700:             implInterruptAll();

Rename implInterruptAll to InterruptAll and remove implInterruptAll.

src/java.base/share/classes/java/util/concurrent/atomic/AtomicIntegerFieldUpdater.java line 399:

> 397:                 if ((ccl != null) && (ccl != cl) &&
> 398:                     ((cl == null) || !isAncestor(cl, ccl))) {
> 399:                     sun.reflect.misc.ReflectUtil.checkPackageAccess(tclass);

The ensureMemberAccess needs to stay but the checkPackageAccess can be removed.

@seanjmullan I think you'll want to study this one closely.

src/java.base/share/classes/java/util/concurrent/atomic/Striped64.java line 385:

> 383:         try {
> 384:             MethodHandles.Lookup l2 =
> 385:                  MethodHandles.privateLookupIn(Thread.class, MethodHandles.lookup());

I assume this can be changed to ` MethodHandles.Lookup l2 = MethodHandles.privateLookupIn(Thread.class, l1);`

-------------

PR Review: https://git.openjdk.org/jdk/pull/22119#pullrequestreview-2437850784
PR Review Comment: https://git.openjdk.org/jdk/pull/22119#discussion_r1843295775
PR Review Comment: https://git.openjdk.org/jdk/pull/22119#discussion_r1843297283
PR Review Comment: https://git.openjdk.org/jdk/pull/22119#discussion_r1843307754
PR Review Comment: https://git.openjdk.org/jdk/pull/22119#discussion_r1843299121
PR Review Comment: https://git.openjdk.org/jdk/pull/22119#discussion_r1843300163
PR Review Comment: https://git.openjdk.org/jdk/pull/22119#discussion_r1843303996
PR Review Comment: https://git.openjdk.org/jdk/pull/22119#discussion_r1843304937