On Thu, 14 Nov 2024 20:45:49 GMT, Roger Riggs <rri...@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
