The default virtual thread executor should created named threads

[danishnawab]

Jetty version(s)
12.0.5

Enhancement Description

Specify a ThreadFactory when setting up the default virtual thread executor so it creates named threads.

Background

The virtual threads created by Jetty's VirtualThreads#getDefaultVirtualThreadsExecutor do not have a thread name.

Unlike platform threads, virtual threads do not return a name unless explicitly set. From the javadoc:

Virtual threads do not have a thread name by default. The getName method returns the empty string if a thread name is not set.

And jetty does not specify a ThreadFactory when creating the Executor internally: link.

The combination of the above two things leads to a reduced troubleshooting experience because things like logs and thread dumps do not contain a useful thread name anymore.

Of course, the consumers could set their own virtual thread executors in VirtualThreads.Configurable#setVirtualThreadsExecutor but that's not what Spring Boot does as it just uses the default executor offered by Jetty: link.

Proposal
Given that Jetty does offer a default executor, it would be good to configure it so it creates named threads.

Now on to the question of what might be a good name in this case: since we don't know the use cases of the consumers, it would be good to give it a generic prefix e.g. jetty-vt-N (where N is the ID incremented internally by the ThreadFactory for each thread it creates).

I don't think it is necessary to allow making this prefix configurable as the consumers could always provide a custom virtual thread executor. (Potentially relevant discussion in #8863)
But with a default (and non-configurable) prefix, we will at least have a sane default in place.

Here's a draft of what such a change might look like: jetty-12.0.x...danishnawab:jetty.project:jetty#11353#diff-0d363273d70f3e11ace94a7a6df43bd1b8d50ffe0629aee6cc3f31805084c047

If these changes are acceptable, I can open a PR for you.

[sbordet]

Please prepare a PR and we'll discuss the changes there.

Note that the reflection code must work for Java 19, 20 and 21 -- as the virtual thread APIs changed a little between these versions, make sure you're not using some API that was there in 19 and removed in 21, or viceversa.

Thanks!

[danishnawab]

I have opened the PR #11355 for this issue.

@sbordet based on your hint, I checked the implementation and it works as expected against JDKs 19 (--enable-preview), 20 (--enable-preview), and 21.
JDK 18 (and older) failed as expected.

Additionally, I compared the JDK docs between versions 19, 20, and 21 and the API used (reflectively) in my PR did not change at all in these versions.

[gregw]

Will this have any performance impacts? I assume virtual threads have no names for a reason, which I assume is to make them as cheap as possible to create (kind of the whole idea of virtual threads). I'd be concerned that adding expense to the creation of a virtual thread is thus fighting against the design of virtual threads.

Perhaps they should only be named if some debug option is configured?

[sbordet]

@gregw every virtual thread will pay the cost of creating the new name, so atomic add and string concatenation.

This is our default virtual thread executor, and can be replaced by applications, but they typically don't (e.g. Spring), but they value observability.

I would not like to use a system property, but perhaps it's the only way.

[danishnawab]

Will this have any performance impacts?

I asked this question on the OpenJDK project loom mailing list: https://mail.openjdk.org/pipermail/loom-dev/2024-February/006417.html

The excerpt from Alan Bateman's response:

To summarize: Virtual threads are intended to be low footprint so they
don't get an automatically generated name by default. They have a thread
ID of course and that thread ID is in the string representation and is
readily available to logging libraries with Thread::threadId. Virtual
threads can be named where it makes sense. In an application with
thousands of virtual threads then it may be useful to name a few special
threads, like the thread that is accepting connections but less useful
to name the 10_000 threads in the same "job role" handling a specific
request. Thread dumps include the thread ID for each thread and will
include the names of threads have have been given a name.

How much of a footprint impact this will have - or if the additional troubleshooting benefits this unlocks are worth the tradeoff - is difficult for me to say. The only solution would be to benchmark the results before and after the proposed change.

What I do know is that the Tomcat API for virtual threads does encourage named virtual threads: https://github.com/apache/tomcat/blob/main/java/org/apache/tomcat/util/threads/VirtualThreadExecutor.java#L39-L41
And the users can make use of that easily: spring-projects/spring-boot@f81787e#diff-b1f85881c3bd0fdd9d4d04c364c0fbe093fb102dbd751565dd88a961629cdacbR39

Of course, an alternative where Jetty users can decide whether or not to take this penalty would be great but the current design of util/VirtualThreads.java doesn't allow that. Maybe instead of a global static defaultExecutor, we should offer a dedicated VirtualThreadExecutor class similar to Tomcat.

In its current form, the alternative is that the users have to provide a custom VirtualThreadExecutor but that's a bit hairy because then library users compiling against older JDKs (e.g. Spring which is compiled against JDK 17) need to do the reflective probe which could otherwise have been nicely abstracted by Jetty: https://github.com/jetty/jetty.project/pull/11355/files#diff-0d363273d70f3e11ace94a7a6df43bd1b8d50ffe0629aee6cc3f31805084c047L41-R47

So the question is if we should configure the default to be debug-friendly (by having named threads) even if that incurs
(a somewhat acceptable) performance penalty. And the performance-sensitive users on the other hand can still provide a custom-configured executor (that doesn't configure a name amongst other performance-focused optimizations).

[sbordet]

Jetty does allow to set a custom virtual threads Executor via QueuedThreadPool.setVirtualThreadsExecutor().

What we can do is to add a VirtualThreads.getDefaultVirtualThreadsExecutor(String namePrefix), so that Spring will be able to use that without having to resort to reflection.

But then Spring needs to make the name prefix easily configurable.

Would the addition of VirtualThreads.getDefaultVirtualThreadsExecutor(String namePrefix) work for you?

[danishnawab]

I can't speak for the Spring team but I don't see a reason why that won't be sufficient.

The interesting part would be how to achieve this on the jetty side. The fact that the namePrefix can be provided from the outside means we can no longer have a "default" executor (cf: getDefaultVirtualThreadsExecutor).
In essence, we will be providing a static factory, which would look something like:

public static Executor newVirtualThreadExecutor(String namePrefix) {
    try
    {
        Class<?> builderClass = Class.forName("java.lang.Thread$Builder");
        Object threadBuilder = Thread.class.getMethod("ofVirtual").invoke(null);
        threadBuilder = builderClass.getMethod("name", String.class, long.class).invoke(threadBuilder, namePrefix, 0L);
        ThreadFactory factory = (ThreadFactory)builderClass.getMethod("factory").invoke(threadBuilder);
        return (Executor)Executors.class.getMethod("newThreadPerTaskExecutor", ThreadFactory.class).invoke(null, factory);
    }
    catch (Throwable x)
    {
        return null;
    }
}

Or do you have a different approach in mind?

[sbordet]

@danishnawab I think we should:

1. Leave the current VirtualThreads.getDefaultVirtualThreadsExecutor() as is.
2. Add VirtualThreads.getNamedVirtualThreadsExecutor(String namePrefix) as a commodity to be used by users/libraries, with javadocs explaining that there is a cost penalty to generate the virtual thread name, but observability benefits.

Would you work on that solution?
Let us know, otherwise we'll take over, keeping your contribution so far.

Thanks!

[danishnawab]

@sbordet thanks for the guidance, sounds good to me.
I will update my contribution accordingly.

[danishnawab]

@sbordet I have opened #11430 based on your suggestions.