QueuedThreadPool should support ThreadFactory behaviors

[joakime]

The JVM class java.util.concurrent.ThreadFactory was introduced in Java 1.5 and the default Executors in Java support it as well.

@sbordet proposes that we have QueuedThreadPool implement java.util.concurrent.ThreadFactory, but also allow the constructor of QueuedThreadPool have an optional ThreadFactory that can be passed in (similar to how the Executors themselves operate)

We should also move the configuration of the new thread ....

https://github.com/eclipse/jetty.project/blob/ba728eee5d9e8cfd3b7449b1246546ca2306bb25/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java#L633-L638

... into the QTP.newThread(Runnable) ...

https://github.com/eclipse/jetty.project/blob/ba728eee5d9e8cfd3b7449b1246546ca2306bb25/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java#L666-L669

[joakime]

The fix in PR #4146 is only a half fix.
I'll submit a followup PR for the rest.

[joakime]

Opened PR #4149 (currently in draft status as I work on unit tests)

[josephlbarnett]

FYI, by making the protected newThread method public, there is some backwards incompatibility here, not sure if that was known/intended. see linked jersey bug for example.

[joakime]

The change was intended.
The QueuedThreadPool constructor takes a ThreadPool now, and will use it.

[gregw]

@josephlbarnett to be precise... the change to use the ThreadFactory API was intended.... that fact that this change breaks backwards compatibility was an unintended consequence. In retrospect we should not have broken that in a point release, but it is a good change and it is out now... we'd cause more disruption I think by reverting.

[NielsDoucet]

@gregw Currently this causes a bug for anyone using the spring-boot-dependencies bom as a baseline for dependency management. See spring-projects/spring-boot#19769.
Is there any way this situation can be improved? Perhaps a special release without the breaking api?

[joakime]

We already have large projects using this new feature (as it was requested by them).

[NielsDoucet]

I understand. As highlighted in the spring-boot ticket linked above, spring-boot clearly also wanted this and other new features, but maybe you have some way to cut a special version of the 9.4.24 release, which rolls back this breaking api change only.

The impact is rather big, as any project using spring boot version 2.2.1+ to drive their dependency management (default behaviour when using their maven or gradle plugin) will automatically get their versions managed to this broken combination of jetty and jersey.

[joakime]

We are about to release 9.4.26 to address issues discovered in 9.4.24 and 9.4.25

Jersey is the reason for the 9.4.26 release due to issue #4461, they will be upgrading to 9.4.26.

[joakime]

@NielsDoucet other large projects that use Jersey have been able to use Jetty 9.4.24 and 9.4.25 without a similar issue to spring-boot.
Why is this unique to spring-boot?

[wilkinsona]

I don't believe it is unique to Spring Boot. As I understand it, it's a problem for anyone trying to use Jersey 2.29.x with Jetty 9.4.22 and later when Jersey's responsible for creating the Jetty instance. As shown in the Boot issue referenced above (spring-projects/spring-boot#19769), one way to do that is by using Jersey's test framework. Spring Boot users who aren't using Jersey's test framework may not be affected at all as Boot will create the Jetty instance and configure Jersey to run on top of it. My understanding is that will take Jersey's org.glassfish.jersey.jetty.JettyHttpContainerFactory$JettyConnectorThreadPool out of the picture so the breaking change to the visibility of newThread won't cause a problem.