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

websocket-next extension should be able to automatically broadcast pings #39862

Closed
edeandrea opened this issue Apr 3, 2024 · 10 comments · Fixed by #40207
Closed

websocket-next extension should be able to automatically broadcast pings #39862

edeandrea opened this issue Apr 3, 2024 · 10 comments · Fixed by #40207
Labels
Milestone

Comments

@edeandrea
Copy link
Contributor

edeandrea commented Apr 3, 2024

Description

This enhancement stems from this conversation: https://quarkusio.zulipchat.com/#narrow/stream/187030-users/topic/websockets-next

A typical WebSocket server will periodically send pings to its clients. A browser client will automatically respond with a pong when it receives a ping.

Since this is something that pretty much every websocket server should do, shouldn't the extension handle this? Other WebSocker server frameworks (like SockJS for example) do this automatically.

Implementation ideas

Maybe its opt-in (config property? annotation? Both?) where the user has control over the interval by which to send the ping message?

@edeandrea edeandrea added the kind/enhancement New feature or request label Apr 3, 2024
@quarkus-bot

This comment was marked as resolved.

@mkouba
Copy link
Contributor

mkouba commented Apr 4, 2024

A typical WebSocket server will periodically send pings to its clients. A browser client will automatically respond with a pong when it receives a ping.

Hm, I'm not an expert here but I think that it should be an opt-in feature because not all servers need it. Also if your application handles a LOT of connected clients then automatic pings may cause significant performance drop.

Since this is something that pretty much every websocket server should do, shouldn't the extension handle this? Other WebSocker server frameworks (like SockJS for example) do this automatically.

Do you have a link where the SockJS implementation/configuration is described?

@gsmet
Copy link
Member

gsmet commented Apr 4, 2024

I think it's worth having a look at how other servers handle this. I know that when I used SSE in Quarkus GitHub App, I had to implement the ping/pong mechanism as otherwise things were VERY unreliable.

@mkouba
Copy link
Contributor

mkouba commented Apr 4, 2024

BTW a simple blocking version of this functionality (after #39858 was merged) implemented using the quarkus-scheduler could look like:

@Inject
OpenConnections connections;

@Scheduled(every = "5s")
void sendPing() {
   Buffer ping = Buffer.buffer("ping");
   for (WebSocketConnection c : connections) {
      c.sendPingAndAwait(ping);
   }
}

@edeandrea
Copy link
Contributor Author

I totally agree it should be opt-in. And the user should have some control over the interval (by config property or something).

@mkouba
Copy link
Contributor

mkouba commented Apr 19, 2024

Since this is something that pretty much every websocket server should do, shouldn't the extension handle this? Other WebSocker server frameworks (like SockJS for example) do this automatically.

Do you have a link where the SockJS implementation/configuration is described?

@edeandrea?

@mkouba
Copy link
Contributor

mkouba commented Apr 19, 2024

CC @cescoffier

@cescoffier
Copy link
Member

@mkouba About sockJS: https://vertx.io/docs/vertx-web/java/#_sockjs.

So, on the cloud, load balancers tend to aggressively recycle TCP connections when they "can" (remember that connection is their concurrency limit). To achieve this, most of them follow a rule: cut the connection if there is no application level frame within 10s.

When implementing an SSE, I use a Multi "merge" trick, sending empty JSON objects (or whatever format) and ignoring them on the client side.

For web sockets, I'm wondering if the ping/pong mechanism is enough. Indeed, it's not at the application level, but at level 4. We should try to reproduce this first.

@mkouba, the openshift sandbox follows the same strategy, so if we can keep a long-running connection just using the ping/pong on the openshift sandbox, we should be good to go.

@mkouba
Copy link
Contributor

mkouba commented Apr 22, 2024

@mkouba About sockJS: https://vertx.io/docs/vertx-web/java/#_sockjs.

So these docs do not say anything about server -> client ping/pong implementation/configuration. Which is what I was trying to find out.

@mkouba, the openshift sandbox follows the same strategy, so if we can keep a long-running connection just using the ping/pong on the openshift sandbox, we should be good to go.

I'll try it and we'll see ;-).

mkouba added a commit to mkouba/quarkus that referenced this issue Apr 23, 2024
- an opt-in config property is used to set the interval
- resolves quarkusio#39862
@mkouba
Copy link
Contributor

mkouba commented Apr 23, 2024

For the record - I tested the quarkus.websockets.next.server.auto-ping-interval with haproxy.router.openshift.io/timeout-tunnel on OpenShift and the auto ping from the server was enough to keep the connection alive.

IMPLEMENTATION NOTE: We do not broadcast the ping but instead schedule a timer for each connection separately.

mkouba added a commit to mkouba/quarkus that referenced this issue Apr 23, 2024
- an opt-in config property is used to set the interval
- resolves quarkusio#39862
mkouba added a commit to mkouba/quarkus that referenced this issue Apr 23, 2024
- an opt-in config property is used to set the interval
- resolves quarkusio#39862
mkouba added a commit to mkouba/quarkus that referenced this issue Apr 24, 2024
- an opt-in config property is used to set the interval
- resolves quarkusio#39862
- rename WebSocketsRuntimeConfig to WebSocketsServerRuntimeConfig
- change the prefix from "quarkus.websockets-next" to
"quarkus.websockets-next.server"
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone Apr 24, 2024
zZHorizonZz pushed a commit to zZHorizonZz/quarkus-grpc-transcode that referenced this issue Apr 24, 2024
- an opt-in config property is used to set the interval
- resolves quarkusio#39862
- rename WebSocketsRuntimeConfig to WebSocketsServerRuntimeConfig
- change the prefix from "quarkus.websockets-next" to
"quarkus.websockets-next.server"
poldinik pushed a commit to poldinik/quarkus that referenced this issue Apr 29, 2024
- an opt-in config property is used to set the interval
- resolves quarkusio#39862
- rename WebSocketsRuntimeConfig to WebSocketsServerRuntimeConfig
- change the prefix from "quarkus.websockets-next" to
"quarkus.websockets-next.server"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants