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

Add support of @RunOnVirtualThread on class for websockets next #44925

Merged

Conversation

Malandril
Copy link
Contributor

This PR allows to configure the virtual thread execution model of the websockets next endpoints on the class with @RunOnVirtualThread .

    @RunOnVirtualThread
    @WebSocket(path = "/chat")
    public class ChatWebsocket {
        @OnTextMessage
        String text(String message) {
          ....
        }
    }

@quarkus-bot quarkus-bot bot added the area/virtual-threads Issue related to Java's Virtual Threads label Dec 4, 2024
@Malandril Malandril force-pushed the websockets-next_virtual_threads_on_class branch from 5a9d85e to 6e9ee38 Compare December 5, 2024 00:01
@gsmet gsmet requested a review from mkouba December 5, 2024 15:00
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

This comment has been minimized.

@Malandril
Copy link
Contributor Author

@mkouba I was wondering also, if it should fail if the annoted Class contains non-blocking methods ?

This comment has been minimized.

This comment has been minimized.

Copy link

github-actions bot commented Dec 5, 2024

🙈 The PR is closed and the preview is expired.

@mkouba
Copy link
Contributor

mkouba commented Dec 9, 2024

@mkouba I was wondering also, if it should fail if the annoted Class contains non-blocking methods ?

Hm, we don't have this validation at the moment. I mean you can annotate a method that returns Uni and we don't fail 🤔. But we can add this validation later.

@@ -240,6 +240,7 @@ WebSocket Next supports _blocking_ and _non-blocking_ logic, akin to Quarkus RES
Here are the rules governing execution:

* Methods annotated with `@RunOnVirtualThread`, `@Blocking` or `@Transactional` are considered blocking.
* All Methods in a class annotated with `@RunOnVirtualThread` are considered blocking and run on a virtual thread.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* All Methods in a class annotated with `@RunOnVirtualThread` are considered blocking and run on a virtual thread.
* Methods declated on a class annotated with `@RunOnVirtualThread` are considered blocking.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also update the bullet about Kotlin suspend functions...

Plus change the last bullet from "Methods annotated with @RunOnVirtualThread must execute on a virtual thread, each invocation spawns a new virtual thread." to something like "Methods annotated with @RunOnVirtualThread or declared on a class annotated with @RunOnVirtualThread must be executed on a virtual thread, each invocation spawns a new virtual thread."

@Malandril Malandril force-pushed the websockets-next_virtual_threads_on_class branch from 4e9f7c6 to 5c4ec9f Compare December 9, 2024 22:13
@Malandril Malandril force-pushed the websockets-next_virtual_threads_on_class branch from 5c4ec9f to 890c787 Compare December 9, 2024 22:18
Copy link

quarkus-bot bot commented Dec 9, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 890c787.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Copy link

quarkus-bot bot commented Dec 9, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 890c787.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@mkouba mkouba merged commit c326bec into quarkusio:main Dec 10, 2024
23 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Dec 10, 2024
@mkouba
Copy link
Contributor

mkouba commented Dec 10, 2024

@Malandril merged, thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation area/virtual-threads Issue related to Java's Virtual Threads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants