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

WebSockets Next: fire CDI events for each connection added/removed #40217

Closed
mkouba opened this issue Apr 23, 2024 · 5 comments · Fixed by #41032
Closed

WebSockets Next: fire CDI events for each connection added/removed #40217

mkouba opened this issue Apr 23, 2024 · 5 comments · Fixed by #41032
Assignees
Labels
Milestone

Comments

@mkouba
Copy link
Contributor

mkouba commented Apr 23, 2024

Description

The payload should contain the connection id.

Implementation ideas

No response

@mkouba mkouba added the kind/enhancement New feature or request label Apr 23, 2024
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Apr 23, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 23, 2024

/cc @Ladicek (arc), @manovotn (arc)

@mkouba mkouba added area/websockets and removed area/arc Issue related to ARC (dependency injection) labels Apr 23, 2024
@mkouba
Copy link
Contributor Author

mkouba commented May 13, 2024

So have several options here. First of all, we should decide if we really want to use CDI events or some other kind of API (listener interface, Vertx eventbus, etc.).

And if we decide to use CDI events, we would need to (A) introduce a special type for each event; e.g. ConnectionOpen/ConnectionClosed and possibly also ClientConnectionOpen/ClientConnectionClosed (the payload would be the identifier assigned to the connection) or (B) introduce open/close qualifiers and reuse the WebSocketConnection and WebSocketClientConnection. We should also decide whether we fire the events synchronously, asynchronously or both.

When I think about it, we could also consider reusing @OnOpen and @OnClose and support global callbacks in a similar way as we support global error handlers, i.e. @OnError declared outside an endpoint... but that could be really confusing 🤔.

@manovotn
Copy link
Contributor

So have several options here. First of all, we should decide if we really want to use CDI events or some other kind of API (listener interface, Vertx eventbus, etc.).

Is there a reason why we shouldn't use CDI events? They seem like a natural fit.

(B) introduce open/close qualifiers and reuse the WebSocketConnection and WebSocketClientConnection.

+1 to this one as it looks simpler.

We should also decide whether we fire the events synchronously, asynchronously or both.

Probably both? If there are no OM for given event, it's pretty cheap anyway.

@mkouba
Copy link
Contributor Author

mkouba commented May 13, 2024

So have several options here. First of all, we should decide if we really want to use CDI events or some other kind of API (listener interface, Vertx eventbus, etc.).

Is there a reason why we shouldn't use CDI events? They seem like a natural fit.

(B) introduce open/close qualifiers and reuse the WebSocketConnection and WebSocketClientConnection.

+1 to this one as it looks simpler.

The only downside I can see is that for "close" events the connection object is basically unusable and most of the operations would result in an exception.

We should also decide whether we fire the events synchronously, asynchronously or both.

Probably both? If there are no OM for given event, it's pretty cheap anyway.

My concern with sync observers is that they block the current thread which might be dangerous because we open/close the connection on the event loop.

@manovotn
Copy link
Contributor

The only downside I can see is that for "close" events the connection object is basically unusable and most of the operations would result in an exception.

Yes, at that point it is just a marker payload.

My concern with sync observers is that they block the current thread which might be dangerous because we open/close the connection on the event loop.

Didn't think about that, that's very good point.

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

Successfully merging a pull request may close this issue.

2 participants