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.

Sign up for GitHub

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

Jump to bottom

WebSockets Next: add support for Kotlin suspend functions #41760

Merged
merged 1 commit into from
Jul 11, 2024
Merged

WebSockets Next: add support for Kotlin suspend functions #41760

merged 1 commit into from
Jul 11, 2024

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jul 8, 2024 •
edited
Loading

Kotlin suspend functions are treated like Java methods that return Uni. That is, they are considered non-blocking. The implementation uses CDI method invokers (to avoid custom bytecode generation), which actually convert the suspend function result into a Uni under the hood.

With this commit, only single-shot suspend functions are supported; suspend functions returning Flow are not supported yet.

Fixes #40325

@Ladicek Ladicek requested review from cescoffier and mkouba July 8, 2024 15:21
@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 8, 2024

This commit changes the websockets-next modules directly, because separating the Kotlin support into a new set of modules or extensions would require massive refactoring.

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/documentation labels Jul 8, 2024
@quarkus-bot quarkus-bot bot added this to To do in Quarkus Documentation Jul 8, 2024
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
cescoffier
cescoffier approved these changes Jul 9, 2024
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

Looks good to me, but let's wait @mkouba's review.

Quarkus Documentation automation moved this from To do to Reviewer approved Jul 9, 2024
mkouba
mkouba reviewed Jul 9, 2024
View reviewed changes
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.

I think that we should update the WS next reference guide as well.

...-next/deployment/src/main/java/io/quarkus/websockets/next/deployment/WebSocketProcessor.java Show resolved Hide resolved
extensions/websockets-next/deployment/pom.xml
<configuration>
<sourceDirs>
<sourceDir>${project.basedir}/src/main/kotlin</sourceDir>
<sourceDir>${project.basedir}/src/main/java</sourceDir>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use the kotlin-maven-plugin to compile the java sources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, honestly. This is how Kotlin Maven plugin is documented (see here) and it's also what we do elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hehe ok. But in our case we don't have kotlin code in src/main/kotlin, or? It's only used in tests. But if we do the same elsewhere 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, we only have Kotlin tests. I figured I could also add Kotlin tests to integration-tests to make the setup simpler, but not sure...

extensions/websockets-next/deployment/pom.xml Show resolved Hide resolved
...ts-next/kotlin/src/main/kotlin/io/quarkus/websockets/next/runtime/kotlin/CoroutineInvoker.kt Show resolved Hide resolved
...-next/deployment/src/main/java/io/quarkus/websockets/next/deployment/WebSocketProcessor.java Show resolved Hide resolved
...ts-next/kotlin/src/main/kotlin/io/quarkus/websockets/next/runtime/kotlin/CoroutineInvoker.kt Show resolved Hide resolved
@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 9, 2024

I think that we should update the WS next reference guide as well.

Good point, let me fix that!

@Ladicek
WebSockets Next: add support for Kotlin suspend functions
2517290 
Kotlin `suspend` functions are treated like Java methods that return `Uni`.
That is, they are considered non-blocking. The implementation uses CDI
method invokers (to avoid custom bytecode generation), which actually
convert the `suspend` function result into a `Uni` under the hood.

With this commit, only single-shot `suspend` functions are supported;
`suspend` functions returning `Flow` are not supported yet.
@Ladicek Ladicek force-pushed the websockets-next-kotlin-suspend-functions branch from 5e4b88f to 2517290 Compare July 10, 2024 09:23
@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 10, 2024

I think that we should update the WS next reference guide as well.

Good point, let me fix that!

Done.

Also rebased and force-pushed.

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 10, 2024

Status for workflow Quarkus Documentation CI

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

✅ 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.

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 10, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 10, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

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

✅ 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.

@Ladicek Ladicek merged commit 6f8c261 into quarkusio:main Jul 11, 2024
55 checks passed
Quarkus Documentation automation moved this from Reviewer approved to Done Jul 11, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jul 11, 2024
@Ladicek Ladicek deleted the websockets-next-kotlin-suspend-functions branch July 11, 2024 07:11
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 11, 2024
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 11, 2024
@github-actions GitHub Actions
Copy link

🙈 The PR is closed and the preview is expired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cescoffier cescoffier cescoffier approved these changes

@mkouba mkouba mkouba approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/documentation area/websockets kind/enhancement New feature or request release/noteworthy-feature triage/flaky-test
Projects
Quarkus Documentation
Done
Milestone
3.13.0.CR1
Development

Successfully merging this pull request may close these issues.

Websockets Next: Kotlin Coroutines Support
4 participants
@Ladicek @cescoffier @mkouba @geoand