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

Jetty 12 - Rewrite of the Jetty WebSocket APIs #9552

Closed
sbordet opened this issue Mar 30, 2023 · 6 comments
Closed

Jetty 12 - Rewrite of the Jetty WebSocket APIs #9552

sbordet opened this issue Mar 30, 2023 · 6 comments
Labels
Enhancement

Comments

@sbordet
Copy link
Contributor

sbordet commented Mar 30, 2023 •
edited
Loading

Jetty version(s)
12+

Enhancement Description
The Jetty WebSocket APIs were designed following a similar design of the Jakarta WebSocket APIs.

However, that design is old and it clashes with other Jetty core APIs that we have.
In particular:

  • No reason to have Session.getRemote()
  • Duplicated blocking and non-blocking APIs
  • The event handling side (WebSocket[Connection|Frame|Partial|PingPong]Listener) has unclear semantic wrt threading. In particular, can an event handler method perform blocking calls?
    Not to mention the number of interfaces for the various events that may not be optimal when it comes to instanceof checks (see https://bugs.openjdk.org/browse/JDK-8180450).
  • Implicit demand for new frames/messages when returning from a method, with Session.suspend() to try to cope with it.
    This clashes with all the other Jetty 12 APIs that have explicit demand APIs.
  • Weird handling of methods that take Reader or InputStream that the implementation treats specially wrt threading: since they offer blocking APIs, there is no demand because the event handler method is not exited, so the implementation has to demand internally in order to feed the Reader or InputStream.

This would be the right time to re-design the APIs to align them with the rest of the Jetty 12 APIs.
@sbordet
Copy link
Contributor Author

sbordet commented Mar 30, 2023

See also #9550 for an explanation of why the undefined semantic of the APIs, coupled with blocking APIs, causes certain scenarios to lockup.

Would be great if WebSocketUpgradeHandler could have back invocationType==NON_BLOCKING because now the APIs have a defined semantic wrt threading (and no possibility to invoke WebSocket blocking APIs).

@sbordet
Copy link
Contributor Author

sbordet commented Mar 30, 2023

@gregw @lachlan-roberts @lorban @joakime your thoughts?

@sbordet sbordet mentioned this issue Apr 3, 2023
Merged
@monish-byte
Copy link

hey @sbordet I would like to work on this issue. As I am a beginner I would appreciate your guidance. thank you.

@lachlan-roberts
Copy link
Contributor

@monish-byte this issue is not something you could help with.

If you want to contribute look at open issues with the "Help Wanted" tag.
https://github.com/eclipse/jetty.project/issues?q=is%3Aopen+is%3Aissue+label%3A%22Help+Wanted%22

@lachlan-roberts
Copy link
Contributor

@sbordet I think all of those points are just because the Jetty API is designed to mirror what the Jakarta WebSocket API is doing.

But I think if we decide to move away from that style now would be a good time.

I would suggest that we preserve the old Jetty WebSocket API for EE8/EE9 and then have a new WebSocket API in jetty-core which is extended in EE10 to give access to the servlet behaviours.

sbordet added a commit that referenced this issue Apr 14, 2023
@sbordet
Fixes #9552 - Jetty 12 - Rewrite of the Jetty WebSocket APIs.
6ae9ad9 
* Removed unnecessary classes, among which BatchMode, CloseStatus, etc.
* Coalesced all listener interfaces into Session.Listener.
* Moved RemoteEndpoint functionality to Session.
* Renamed WebSocketPolicy to Configurable.
* Renamed WriteCallback to just Callback, as it is now also used for some listener methods.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet mentioned this issue May 2, 2023
Open
sbordet added a commit that referenced this issue May 2, 2023
@sbordet
Fixes #9552 - Jetty 12 - Rewrite of the Jetty WebSocket APIs. (#9652)
adf5754 
* Removed unnecessary classes, among which `BatchMode`, `CloseStatus`, etc.
* Coalesced all listener interfaces into `Session.Listener`.
* Moved `RemoteEndpoint` functionality to `Session`.
* Renamed `WebSocketPolicy` to `Configurable`.
* Renamed `WriteCallback` to just `Callback`, as it is now also used for some listener methods.
* Renamed `@OnWebSocketConnect` to `@OnWebSocketOpen`
* Renamed `Session.Listener.onWebSocketConnect()` to `onWebSocketOpen()`.
* Removed `@WebSocket` annotation attributes, because they were just a subset of the configurable ones and they can be configured directly on the Session instance.
* Removed `Session.suspend()` and `SuspendToken`, and introduced `Session.demand()`.
* Introduced `@WebSocket.autoDemand` and `Session.Listener.AutoDemanding` to support automatic demand upon exit of WebSocket handler methods.
* Removed `FrameHandler.isAutoDemanding()` and `CoreSession.isAutoDemanding()`.
* Changed the responsibility of demand from `WebSocketCoreSession` to `FrameHandler`, which in turn may delegate to `MessageSink`.
* Updated MessageInputStream to fail if an exception is thrown by the application.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Copy link
Contributor Author

sbordet commented May 2, 2023

Fixed by #9652.

@sbordet sbordet closed this as completed May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sbordet @lachlan-roberts @monish-byte