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

QoSHandler does not resume on a virtual thread #12171

Closed
sbordet opened this issue Aug 19, 2024 · 4 comments · Fixed by #12174
Closed

QoSHandler does not resume on a virtual thread #12171

sbordet opened this issue Aug 19, 2024 · 4 comments · Fixed by #12174
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@sbordet
Copy link
Contributor

sbordet commented Aug 19, 2024

Jetty version(s)
12

Description
See #12154 (comment).

When QoSHandler resumes a request that was dispatched on a virtual thread, it is resumed on a non-virtual thread.

@sbordet sbordet added the Bug For general bugs on Jetty side label Aug 19, 2024
@sbordet sbordet self-assigned this Aug 19, 2024
sbordet added a commit that referenced this issue Aug 19, 2024
Now QoSHandler attempts to resume using a virtual thread if the request was handled with a virtual thread and then suspended.

Removed warn() from VirtualThreads.isVirtualThread(), it was too verbose.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@gregw
Copy link
Contributor

gregw commented Aug 19, 2024

Firstly, I don't think this should be an issue. An application should not care if it is given a real thread or a virtual thread. In our normal virtual thread mode, there will always be some normal threads used for callbacks and these can be further dispatched into the application.

If a user really cares about this, then they should use the all-virtual mode and then hopefully the "ThreadPool" returned by getServer().getThreadPool() would execute in a virtual thread.... if not then it should.

Perhaps in normal virtual thread mode we want a simple method that will do the same logic as our strategy (virtual thread for blocking invocations, just call otherwise)?

@sbordet
Copy link
Contributor Author

sbordet commented Aug 20, 2024

In our normal virtual thread mode, there will always be some normal threads used for callbacks and these can be further dispatched into the application.

@gregw no, we are quite precise with the above, and we have tests that prove that all callbacks to applications are actually performed in virtual threads, even when using the "mixed" mode thread pool, see e.g. https://github.com/jetty/jetty.project/blob/jetty-12.0.12/jetty-ee10/jetty-ee10-tests/jetty-ee10-test-client-transports/src/test/java/org/eclipse/jetty/ee10/test/client/transport/VirtualThreadsTest.java#L85.

In this view, this issue and related PR make sense, as applications may want to use virtual threads to be invoked, but they also do not want to pin carriers in NIO's select() when using the server.

As for the method to capture the logic, I can add a method VirtualThreads.Configurable.tryVirtualExecute(Runnable task), but the additional logic of AdaptiveExecutionStrategy (checking if the task is BLOCKING), as well as the additional logic of QoSHandler (checking if the task was invoked using a virtual thread) will remain in those classes respectively.

tryVirtualExecute(Runnable) would just do:

if (_virtualExecutor != null)
    virtualExecutor.execute(task);
else 
    _executor.execute(task);

@gregw
Copy link
Contributor

gregw commented Aug 20, 2024

@sbordet I think that test is only passing because the callbacks are blocking. If a core handler sets a demand callback that was non blocking, then it mm ay be called with a native thread.

It just so happens that all callbacks in servlets are seen as blocking.

I don't think apps should be making them decision. They should just can execute on the thread pool and let them configuration make them decision.

@sbordet
Copy link
Contributor Author

sbordet commented Aug 20, 2024

It just so happens that all callbacks in servlets are seen as blocking.

It does not happen, we made it on purpose, because it is application code that we don't know what it does.

I don't think apps should be making them decision. They should just can execute on the thread pool and let them configuration make them decision.

Not sure I follow. The configuration is "call apps with a virtual thread", so if we don't it's a bug.

sbordet added a commit that referenced this issue Aug 26, 2024
Now QoSHandler attempts to resume using a virtual thread if the request was handled with a virtual thread and then suspended.

Removed warn() from VirtualThreads.isVirtualThread(), it was too verbose.

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
Bug For general bugs on Jetty side
Projects
No open projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants