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

Initial version of the new declarative WebSocket server API #39142

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Mar 4, 2024

This pull request supersedes #38783.

@mkouba mkouba requested review from cescoffier, Ladicek and geoand March 4, 2024 08:23
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file labels Mar 4, 2024
ConcurrencyLimiter(WebSocketServerConnection connection) {
this.connection = connection;
this.uncompleted = new AtomicLong();
this.queueCounter = new AtomicLong();
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory , if you need telemetry; you could just use the uncompleted one which can report the number of in-flight requests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The queueCounter is currently only used to distinguish queued actions in log messages. So I don't think we can use uncompleted there. In theory, we could initialize the queueCounter only if Logger.isDebugEnabled(). However, I do remember some problems with native image as described in #27735.

This comment has been minimized.

@gastaldi
Copy link
Contributor

gastaldi commented Mar 4, 2024

@mkouba nice! One question: Does it support PING/PONG messages OOTB? This is described here: #31157

@mkouba
Copy link
Contributor Author

mkouba commented Mar 4, 2024

@mkouba nice! One question: Does it support PING/PONG messages OOTB? This is described here: #31157

We don't add anything special but AFAIK Vert.x does respond to a PING frame sent from the client automatically.

We might need to add something for the server -> client communication though (to ensure the client is still responsive), i.e. add WebSocketServerConnection#sendPing() or so.

CC @cescoffier

@cescoffier
Copy link
Member

Yes,

Normally pong are handled automatically. However, we may want to add the possibility to send them explicitly.

I'm wondering if this can be used to avoid being cut on OpenShift/Kubernetes after 10s of inactivity.

@geoand
Copy link
Contributor

geoand commented Mar 5, 2024

websockets-next needs to be after this.

@mkouba
Copy link
Contributor Author

mkouba commented Mar 5, 2024

websockets-next needs to be after this.

Good point! 👍

@mkouba mkouba force-pushed the websockets-next-final branch from 3a60915 to 8c7286e Compare March 5, 2024 07:44

This comment has been minimized.

@mkouba mkouba force-pushed the websockets-next-final branch from 8c7286e to e09a769 Compare March 5, 2024 08:40
@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation labels Mar 5, 2024
@geoand geoand removed area/documentation area/arc Issue related to ARC (dependency injection) labels Mar 5, 2024
@mkouba
Copy link
Contributor Author

mkouba commented Mar 5, 2024

@Ladicek @geoand Guys I appreciate your comments about javadoc and I'll incorporate them in this PR but keep in mind that the javadoc and docs in general is still WIP (docs do not exist yet actually ;-) and will require a lot of refinements. That said, the goal of this PR is to introduce a minimum viable product version of the API.

PS. The same applies to tests.

@Ladicek
Copy link
Contributor

Ladicek commented Mar 5, 2024

I'm not capable of reviewing this in full, but I went through some parts that felt important (or interesting). I vaguely recall I had some ideas about codecs, but I no longer recall. Maybe I'll remember later.

One thing I wanted to ask: what happens if we (at some point) introduce a CDI session context for Vert.x Web sessions (imagine only for the in-memory, non-clustered scenario, where the Vert.x Web session can store arbitrary objects)? Will it clash with the WebSocket session context added here?

@mkouba
Copy link
Contributor Author

mkouba commented Mar 5, 2024

One thing I wanted to ask: what happens if we (at some point) introduce a CDI session context for Vert.x Web sessions (imagine only for the in-memory, non-clustered scenario, where the Vert.x Web session can store arbitrary objects)? Will it clash with the WebSocket session context added here?

That's a good question. I think that it would depend on how and when we activate the session context. In theory, we only care about the handshake request in case WebSockets. So I assume that if the route handler of a WS endpoint has higher priority than the handler that activates the Vert.x Web session context then we're fine. And ofc it would mean that those two session contexts are completely separated. Which is IMO fine if properly documented.

@mkouba mkouba force-pushed the websockets-next-final branch from e09a769 to d73514d Compare March 5, 2024 11:06
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation labels Mar 5, 2024
@geoand
Copy link
Contributor

geoand commented Mar 5, 2024

Guys I appreciate your comments about javadoc and I'll incorporate them in this PR but keep in mind that the javadoc and docs in general is still WIP

That's why my comment explicitly mentioned the next iteration :)

@mkouba mkouba merged commit 1a4bab8 into quarkusio:main Mar 5, 2024
49 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Mar 5, 2024
Copy link

quarkus-bot bot commented Mar 5, 2024

Status for workflow Quarkus CI

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

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


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 extensions/websockets-next/server/deployment

io.quarkus.websockets.next.test.broadcast.BroadcastOnMessageTest.testUpMultiBidi - History

  • expected: <true> but was: <false> - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:31)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:183)
	at io.quarkus.websockets.next.test.broadcast.BroadcastOnMessageTest.assertBroadcast(BroadcastOnMessageTest.java:96)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/websockets release/noteworthy-feature triage/flaky-test
Projects
Development

Successfully merging this pull request may close these issues.

6 participants