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

Enable support for project loom for Jetty's thread pool #7457

Merged
merged 4 commits into from Oct 18, 2023

Conversation

zUniQueX
Copy link
Member

This PR provides a new configuration option to enable virtual threads for Jetty's thread pool.

If the current runtime doesn't support virtual threads, platform threads are used instead.

@zUniQueX
Copy link
Member Author

@dropwizard/committers This implementation should be enough to enable virtual threads. For basic testing we could ship with this implementation and add support for virtual thread executors in the dropwizard-lifecycle module later IMHO.

Blocked by dropwizard/metrics#3531

@zUniQueX zUniQueX marked this pull request as ready for review September 26, 2023 18:08
@zUniQueX zUniQueX requested a review from a team as a code owner September 26, 2023 18:08
@zUniQueX zUniQueX removed the blocked label Sep 26, 2023
@mattnelson
Copy link
Contributor

Do any of the other thread based properties need to be handled/documented differently when virtual is enabled?

  • maxThreads
  • minThreads
  • adminMaxThreads
  • adminMinThreads
  • idleThreadTimeout
  • acceptorThreads
  • selectorThreads

@cty123
Copy link

cty123 commented Oct 16, 2023

Thanks for working on supporting project loom, is there a plan to backport this change to release 3.x?

@zUniQueX
Copy link
Member Author

Hi @cty123. As mentioned in the release notes, Dropwizard 3.x and 4.x should be kept in sync. Since the release/4.0.x branch is the default one, ongoing development is targeted there. After the merge the changes from 4.x will be backported to 3.x.

@zUniQueX
Copy link
Member Author

@dropwizard/committers Anyone available for this PR?

@zUniQueX
Copy link
Member Author

Do any of the other thread based properties need to be handled/documented differently when virtual is enabled?

* maxThreads

* minThreads

* adminMaxThreads

* adminMinThreads

* idleThreadTimeout

* acceptorThreads

* selectorThreads

minThreads, maxThreads and idleTimeout are set with the constructor of the main InstrumentedQueuedThreadPool and will use virtual threads now from Jetty's default thread pool.

acceptorThreads and selectorThreads obtain threads from the same thread pool and therefore will also use virtual threads.

For the admin connector the InstrumentedQueuedThreadPool doesn't set a ThreadFactory yet and therefore won't use virtual threads. I'm not sure, if applications will benefit much from virtual threads on the admin connector. But we can discuss introducing a separate property for this use case.

Copy link
Contributor

@pstackle pstackle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, LGTM.

I do wonder if we shouldn't fail more obviously in the case that the user configures enableVirtualThreads: true and it can't work (either isn't supported or the reflection code fails). It seems like Dropwizard's typical approach is to fail rather than quietly ignore a config setting (beyond a warn log statement).

@zUniQueX
Copy link
Member Author

@pstackle Thanks for the fast review!

I wanted to introduce the logs only for an easier migration path. But I agree with you, that users should be notified more than that, if the application cannot do what's requested. I've revised the PR and the code now throws exceptions.

I additionally added support for virtual threads on the admin connectors. Then the behavior of the connectors is more similar. But for the separate thread pool there is a separate configuration option to enable it.

Do you think we should provide any other documentation than the configuration options? I'm not sure it's a topic this big since most users will most likely not directly migrate to Java 21 and make use of virtual threads. The interested users probably have already seen the issue and this pr 😉 If you think so, I'll leave a short note in the core docs.

Copy link
Contributor

@pstackle pstackle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one nitpick in an exception message, but otherwise, I'm happy with this.

@pstackle
Copy link
Contributor

I think this feature is likely one that is worth calling out specifically in the release notes, because this is one of the main features in Java 21. I can't really think of a place in the other docs that it would make sense to reference this. Especially because we'd have to caveat it with that you must be running Java 21 or greater to even enable it.

@zUniQueX
Copy link
Member Author

Sounds good 👍

@zUniQueX zUniQueX enabled auto-merge (squash) October 18, 2023 17:01
@zUniQueX zUniQueX merged commit 5fbcb6d into dropwizard:release/4.0.x Oct 18, 2023
11 of 12 checks passed
@zUniQueX zUniQueX deleted the support-loom branch October 18, 2023 18:11
zUniQueX added a commit to zUniQueX/dropwizard that referenced this pull request Oct 18, 2023
* Enable support for project loom for Jetty's thread pool

* Add support for virtual threads on the admin connectors

Co-authored-by: Peter Stackle <pstackle@users.noreply.github.com>

---------

Co-authored-by: Peter Stackle <pstackle@users.noreply.github.com>

Refs dropwizard#7457
(cherry picked from commit 5fbcb6d)
zUniQueX added a commit that referenced this pull request Oct 18, 2023
* Enable support for project loom for Jetty's thread pool

* Add support for virtual threads on the admin connectors

Co-authored-by: Peter Stackle <pstackle@users.noreply.github.com>

---------

Co-authored-by: Peter Stackle <pstackle@users.noreply.github.com>

Refs #7457
(cherry picked from commit 5fbcb6d)
@joschi joschi linked an issue Oct 19, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Project Loom in Jetty 10 & JDK 17+
4 participants