search

jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.86k stars 1.91k forks source link

Correct PrivilegedThreadFactory javadoc #12430

Closed stoty closed 2 weeks ago

stoty commented 2 weeks ago

Jetty version(s) 12.0 12.1

Jetty Environment all

Java version/vendor (use: java -version) JDK 18+

OS type/version any

Description PrivilegedThreadFactory was added as a workaround for a bug which was fixed in JDK18. Using it on later JVMs adds a small overhead, and may cause error or warning messages to be logged, due to the SecurityManager removal.

Use the default ThreadFactory implementation on JDK18+. This could be done by directly checking the JVM version, or by using a multi-release jar.

How to reproduce?

janbartel commented 2 weeks ago

I'm not sure the problem is not present in more recent JDKs. Looking back at the initial issue for which PrivilegedThreaadFactory was created #5859 we see that one of the problems is a field called inheritedAccessControlContext of type AccessControlContext which is still present and initialized even in JDK23. Can you link to a definitive bug resolution in the JDK for this problem? If not, I think we have to keep PrivilegedThreadFactory.

stoty commented 2 weeks ago

I took this information from comment the Jetty source code :

https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java

I also got this information from @joakime in https://github.com/jetty/jetty.project/issues/12318#issuecomment-2376927917

I did not independently verify that the bug is fixed.

stoty commented 2 weeks ago

Now that I re-read those, there is actually a discrepancy, as in the issue comment JDK 17 is mentioned as not having this bug, while in the source comment JDK18 is mentioned as the bug-free version.

stoty commented 2 weeks ago

Digging around in the JDK bug DB, it seems that original issue has not been fixed indeed. JDK has instead added similar workarounds for a lot of its internal ThreadFactory implementations. (i.e. https://github.com/openjdk/jdk/blob/54327bc4e38773b7461977ce17f2185c068bce9b/src/java.base/share/classes/java/util/concurrent/ForkJoinPool.java#L1132 )

I apologize.

However, if the bug is not fixed, then the misleading comment should be fixed PrivilegedThreadFactory. Looks like the comment conflates the SecurityManagar deprecation/removal with the original issue.

janbartel commented 2 weeks ago

I've opened PR #12440 to update the erroneous comment.

stoty commented 2 weeks ago

Thank you. Sorry for the ill-researched issue, but at least the confusion has been cleared up.