Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a possibility to name virtual threads #8863

Closed
dzikoysk opened this issue Nov 3, 2022 · 8 comments · Fixed by #8903
Closed

Provide a possibility to name virtual threads #8863

dzikoysk opened this issue Nov 3, 2022 · 8 comments · Fixed by #8903
Assignees

Comments

@dzikoysk
Copy link

dzikoysk commented Nov 3, 2022

Enhancement Description

Jetty 11 supports Loom, but we don't have any control over the used executor:

https://github.com/eclipse/jetty.project/blob/1e0ef57bb4656b7e5e29bf38b1c7a2772e2d0a2a/jetty-util/src/main/java/org/eclipse/jetty/util/VirtualThreads.java#L34-L41

We should be able to customize executor using OfVirtual:

To e.g. name virtual threads (currently it looks like VirtualThread[#349]/runnable@ForkJoinPool-1-worker-5). Would be nice, if Jetty could provide an abstraction over this builder or just provide a possibility to manually assign customized instance. Currently it's not possible, because this field is final, so some kind of a setter could be an option.

@gregw
Copy link
Contributor

gregw commented Nov 3, 2022

A setter on VirtualThreads is not sufficient, as the executor there is static and if you are naming virtual threads, you may wish to have different names for different execution strategies.

The solution may be to have a setVirtualThreadExecutor on the AdaptiveExecutionStrategy, but the style of usage that we have for other options is to look for a sub interface on the passed Executor... which we already do to some extent, looking for VirtualThreads.Configurable. So maybe we need a VirtualThreads.VirtualExecutor interface???? @sbordet thoughts?

@gregw
Copy link
Contributor

gregw commented Nov 3, 2022

Or maybe a setter for the name on the VirtualThreads.Configurable interface and it would wrap the executor to set the name? Or maybe a VirtualThreads.Configurable.setCustomizer ???

@sbordet
Copy link
Contributor

sbordet commented Nov 4, 2022

@gregw @dzikoysk I don't want to commit Jetty to Java preview APIs that could go on for years and change considerably in every Java version, so we would have Jetty methods that work on Java 19, but don't work on Java 20, but maybe they work again on Java 21, who knows.

We have already done this for the Java foreign APIs and it's a different Jetty module for every Java version.

The current approach is to do the minimum possible to support virtual threads, with minimal exposure to users. For now it is just a boolean useVirtualThreads.

When virtual threads will be finalized, I'm all for better configurability, but for now I'd like to have the minimum possible exposure.

@dzikoysk
Copy link
Author

dzikoysk commented Nov 4, 2022

If you'd open access to this field a little bit (by setter or sth) it'd be enough for me to inject my instance of Executor and you don't have to care about reflections as it's up to me to provide whatever I want that implements Executor here.

@sbordet
Copy link
Contributor

sbordet commented Nov 4, 2022

Having a setter for an Executor opens the possibility of misuse though -- as you can set a non-virtual Executor and therefore there will be no virtual threads, leading to maximal confusion 😄

What exact problem do you have with thread names?
Would it be ok if the thread names are generated by Jetty?

@dzikoysk
Copy link
Author

dzikoysk commented Nov 4, 2022

If someone will pass other executor impl to setter called setVirtualThreadExecutor then I'm not sure if it's really your problem 😅 That's kinda a design choice on Java side to use the same interface, personally, I wouldn't be confused by that.

We'd like to migrate to built-in Loom support in QueuedThreadPool in Javalin:

So we can reduce impl on our side + return QueuedThreadPool (that implements Lifecycle with start/stop) instead of ThreadPool instance.

sbordet added a commit that referenced this issue Nov 16, 2022
Reworked the VirtualThreads APIs to be based on Executor rather than just boolean.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet linked a pull request Nov 16, 2022 that will close this issue
@sbordet
Copy link
Contributor

sbordet commented Nov 16, 2022

@dzikoysk can you try branch fix/jetty-10-configure-virtual-threads and see if it fits your needs?

@joakime joakime moved this from To do to 🔖 Review in progress in Jetty 10.0.13 / 11.0.13 - FROZEN Nov 16, 2022
sbordet added a commit that referenced this issue Nov 18, 2022
Reworked the VirtualThreads APIs to be based on Executor rather than just boolean.

Signed-off-by: Simone Bordet <[email protected]>
Repository owner moved this from 🔖 Review in progress to ✅ Done in Jetty 10.0.13 / 11.0.13 - FROZEN Nov 21, 2022
@dzikoysk
Copy link
Author

Thanks, I like that I can pass my own instance of Executor now, so it solves that issue for me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants