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

Support Programmatic WebSocket upgrade in Jetty 10 #5866

Closed
lachlan-roberts opened this issue Jan 8, 2021 · 12 comments · Fixed by #5874
Closed

Support Programmatic WebSocket upgrade in Jetty 10 #5866

lachlan-roberts opened this issue Jan 8, 2021 · 12 comments · Fixed by #5874
Assignees

Comments

@lachlan-roberts
Copy link
Contributor

Jetty version
10.0.x

Description
Issue brought up on the jetty-dev mailing list by @rstoyanchev.

I need to support WebSocket upgrades on Jetty 10 from within a web framework. Like what JettyWebSocketServlet or WebSocketUpgradeFilter do internally but not actually using them nor relying on their mapping mechanism.

The upgrade needs to be performed within a web framework which has its own Servlet, mappings, and infrastructure so it's not feasible to extend JettyWebSocketServlet. In addition, WebSocket upgrades are supported on multiple servers from the same place in the code which necessitates a programmatic API. Last but not least, the framework can also run on an abstraction layer above the Servlet API, with support for non-Servlet containers, which makes it very challenging to devise a solution based on use of a Servlet or Filter.

@lachlan-roberts lachlan-roberts self-assigned this Jan 8, 2021
lachlan-roberts added a commit that referenced this issue Jan 8, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@sbordet
Copy link
Contributor

sbordet commented Jan 8, 2021

@lachlan-roberts please consider that this seems the same use case that is already implemented in CometD, so perhaps we don't need anything because it's already there?

@lachlan-roberts
Copy link
Contributor Author

@sbordet I think CometD is still using the WebSocketUpgradeFilter, and it would be registering the mappings through the JettyWebSocketServerContainer.

@sbordet
Copy link
Contributor

sbordet commented Jan 8, 2021

@lachlan-roberts the upgrade mechanism is a standard in Servlet APIs, so I'm not sure what exactly is that we can't already do?

And if we really want to use a Jetty specific mechanism, WebSocketMappings.upgrade() is not enough?

@rstoyanchev
Copy link

@sbordet my request is for a programmatic way to perform a WebSocket upgrade at runtmie without having to register mappings. Those mappings are already maintained independently.

@sbordet
Copy link
Contributor

sbordet commented Jan 8, 2021

@rstoyanchev did you try the standard Servlet 4.0 API for upgrade? Not good/enough for your case?

@rstoyanchev
Copy link

rstoyanchev commented Jan 8, 2021

The only thing I'm aware of is the upgrade method on HttpSevletRequest but if IIRC that is a low level hook created for JSR-356 to get access to the underlying connection while the Servlet API remains unaware of any further details. Is there anything else I'm not aware of? I want to upgrade and handle the WebSocket with an @org.eclipse.jetty.websocket.api.annotations.WebSocket handler.

@gregw
Copy link
Contributor

gregw commented Jan 9, 2021

@sbordet we don't support the standard upgrade mechanism. Plus it won't upgrade to our websocket implementation even if we did.

lachlan-roberts added a commit that referenced this issue Jan 12, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

@rstoyanchev PR #5874 is ready if you want to take a look and give some feedback for your use case.

This exposes some additional classes including Handshaker in the websocket-core API. However you should avoid using websocket-core directly if possible. We have added a way to do programmatic websocket upgrades directly using the Jetty WebSocket API.

JettyWebSocketServerContainer container = JettyWebSocketServerContainer.getContainer(getServletContext());
...

JettyWebSocketCreator creator = (req, resp) -> new EchoSocket();
container.upgrade(creator, request, response);

@rstoyanchev
Copy link

@lachlan-roberts this looks good from an initial look. I will perform a more complete experiment next to confirm but as an API this is what I was looking for.

@rstoyanchev
Copy link

I have now done a deeper experiment successfully. I still need to do some more testing on the async side but overall I can confirm the API is good for our needs.

Note that I did run into other challenges due to types that have moved or changed, e.g. Frame, ExtensionConfig, WebSocketPolicy. This a challenge for us because we'll support both Jetty 9 and 10 for a while. I've been able to work around most of those via reflection but some remain a challenge like having an @OnWebSocketFrame method that can work in both Jetty 9 and 10. We use it to enable support for access to Pong messages, so it's not a high priority, but worth mentioning.

Thanks for these changes!

@joakime
Copy link
Contributor

joakime commented Jan 14, 2021

@rstoyanchev lots of internals changes from Jetty 9 to Jetty 10 (and just as big from Jetty 10 to Jetty 11)

@sunng87
Copy link
Contributor

sunng87 commented Jan 19, 2021

I'm working on a Clojure websocket server based on jetty too. As in Jetty 9, it relies on WebSocketHandler to configure websocket policy and create the creator

As I checked demos of Jetty 10, the new APIs requires subclassing JettyWebSocketServlet and give the servlet class to ServletContextHandler. The issue with this API is lack of ability to configure the servlet at runtime.

Being able to call container.upgrade(creator, request, response); can help a lot on this issue.

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

Successfully merging a pull request may close this issue.

6 participants