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

Fixes #8863 - Provide a possibility to name virtual threads #8903

Merged
merged 5 commits into from
Nov 21, 2022

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Nov 16, 2022

Reworked the VirtualThreads APIs to be based on Executor rather than just boolean.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

@sbordet sbordet requested review from gregw, joakime and lorban November 16, 2022 15:56
@sbordet sbordet linked an issue Nov 16, 2022 that may be closed by this pull request
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

Do we have a release with the old method names?
If so, this can't be removed, only deprecated.

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I don't love it, as it is similar yet different to the pattern we've applied for TryExecutor.
Was there a reason we didn't just invent our own interface like:

interface VirtualExecutor
{
    void executeVirtually(Runnable task);
}

So we could follow the pattern of TryExecutor ?

Comment on lines 474 to 472
Executor executor = _virtualThreadsExecutor;
if (executor == null)
executor = _executor;
executor.execute(task);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a little strange that we have both an _executor and a _virtualThreadsExecutor, but use one or the other.

I guess we need the _executor when running with virtual threads and reserved threads, so the reserved threads can be started by the normal executor.

It would be nice if it could more closely resemble the _tryExecutor pattern.

@sbordet
Copy link
Contributor Author

sbordet commented Nov 17, 2022

@gregw TryExecutor is an "internal" abstraction -- we do not allow users to set their own TryExecutor on an existing thread pool.

For virtual threads is different, we want to give users a way to pass their own virtual Executor which they can build how they want (e.g. with names, with ThreadLocals support, and all the properties that Thread.Builder supports).

We want also to support a "simple" way to configure this in Jetty modules.
Right now it's a simple boolean, but feels like we should have instead a threadpool-virtual.mod instead where we can configure the virtual Executor (with names, etc.).

WRT to AdaptiveExecutionStrategy, it's really an if/else: if virtual threads are enabled do this, else do that.
Before it was done with a boolean.

Even if from AdaptiveExecutionStrategy we call only executeVirtually(task), the QueuedThreadPool would have to do the if/else and delegate either to a virtual Executor, or to a itself as a non-virtual Executor.

I think we need 2 Executors because we want to give different names, and it's not only the names: the virtual Executor can be configured to not support ThreadLocals, etc.

Now <Call>, <Get> and <Set> elements can use the "class" attribute
to specify the exact class to perform method lookup.

Improved support for <Property>, <SystemProperty> and <Env> so that
attribute "name" is now optional (as specified in the DTD), and a
"deprecated" attribute may be present instead.
This is necessary to terminally deprecate properties that have
no replacement.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Reworked the VirtualThreads APIs to be based on Executor rather than just boolean.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet force-pushed the fix/jetty-10-configure-virtual-threads branch from 4e1eac4 to 01ca480 Compare November 18, 2022 18:42
@sbordet sbordet merged commit 83154b4 into jetty-10.0.x Nov 21, 2022
@sbordet sbordet deleted the fix/jetty-10-configure-virtual-threads branch November 21, 2022 14:39
sbordet added a commit that referenced this pull request Nov 21, 2022
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a possibility to name virtual threads
3 participants