-
Notifications
You must be signed in to change notification settings - Fork 133
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
FELIX-6692 Add Jetty WebSocket support for jetty 11.x #309
Conversation
Lgtm. Fyi: You may want to join the spec call. |
This is a very nice solution. Would be worthwhile to see if this also works on 12, as this code is a lot neater in terms of initialization. Can you list the jetty bundles that are required to be deployed in the OSGi environment? |
@paulrutter Thanks for reviewing and providing feedback. I haven't tried this with jetty12, but the documentation looks similar with 2 additional implementations (Jakarta EE 8 and Jakarta EE 10). After reviewing the jetty12 documentation, I will rename the "org.apache.felix.jakarta.websocket.enable" option to "org.apache.felix.jakarta.ee9.websocket.enable" to avoid conflicts if we decide to go with this solution and expand it to support the other two options for jetty12. Regarding the jetty bundles that are required, below is what I believe is the minimum set of additional bundles to satisfy the dependencies: For the jetty specific websocket implementation
For the jakarta EE9 specific websocket implementation
|
@enapps-enorman could you add an example to the whiteboard sample project? It would help understand the solution and annotations needed. https://github.com/apache/felix-dev/tree/master/http/samples/whiteboard You might need to create a separate project as the current one relies on the jetty12 bundle. |
@paulrutter With my last commit f256aab I added some paxexam integration tests to verify the functionality. Is that a good enough sample to demonstrate how to use it? |
It would be interesting to see if this approach would also work for the jetty12 bundle, although i'm not a fan of needing to install all those jetty bundles in the OSGi runtime. But that's a matter of taste i suppose. |
Yes that could be interesting, but I am not going to spend any time on experimenting with jetty12 until I get a yes or no regarding whether there is any chance that this proposal is going to be accepted or not. The source code is available here if you want to try something similar on jetty12 and tell us if it worked. FYI: In my projects, I am using the "light" classifier variant of the http.jetty bundle which doesn't embed the classes from any of the jetty bundles so I am used to declaring all of the individual jetty bundles that I need. I prefer this so I have the flexibility to switch to a newer release of jetty without waiting for felix to do a new release with the newer version of the jetty classes embedded. If the consensus is to embed the content of the extra websocket-* bundles into the non-light variant then I would not care too much either way. It would just make the binary a bit bigger. |
Yes, i understand. I think @cziegeler is the one that works on the http part regularly. He can give some more guidance maybe. Since the websocket change for jetty12 is not released yet, this is the moment to check alternatives. |
Thanks @enapps-enorman , I like this approach as it is not enlarging the felix http jetty bundle. |
I will try it out @cziegeler, maybe we should wait with releasing a new version of jetty12 until we know it's feasible or not. If the approach would work with Jetty12, i would propose to do the following:
|
@enapps-enorman @cziegeler what do you think of this? #310 This way, both styles are supported:
The PR doesn't contain tests yet, but if we align on the approach, i could do that based on the work from @enapps-enorman. |
If we go along with this PR and #310, i think the following changes would need to be made to this PR:
|
Rename org.apache.felix.jetty.websocket.enable to org.apache.felix.jetty.ee9.websocket.enable Incorporate the changes for using maybeStoreWebSocketContainerAttributes
@paulrutter Thanks for working out the jetty12 stuff. I've made an attempt to integrate the code changes you suggested. |
The changes look good, only thing left would be to update the documentation to include the new settings. Update the README to document the new options for the Jetty 11 bundle (see https://github.com/apache/felix-dev/pull/310/files#diff-8e568763d7d4e15f9465300afa93c121c7f8e399acfff4c2aee7e70ed1199401) |
I could do that but I guess I was thinking that it might be an awkward to merge with two PRs making changes to that file. I believe at this point the changes in #309 and #310 are changing different files (or identical changes to the same file). Would it be easier to do a followup PR for the README after the other two are merged? |
We could do that, or i merge in your branch into mine and make the changes there? |
* Revert "Add jetty websocket support to Jetty12 (#298)" This reverts commit 6d95c93. * FELIX-6692 Add Jetty WebSocket support for jetty 12.x - Apply 11.x approach to jetty12 bundle - Add two new classifiers 'with-jetty-ee10-websockets' and 'with-jakarta-ee10-websockets' to have a fat jar containing the appropriate websocket classes * Working example base on previous code. Do note that the workaround in FelixJettyWebSocketServlet are still required; the initialization code in the Jetty12 bundle doesn't seem to work * * Enable cross context support to allow WebSockets to be registered in Jetty 12. See https://github.com/jetty/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java#L510 and jetty/jetty.project#9774 * Add example based on registering the websocket to the main servlet context instead of the per-bundle one - * Added paxweb unit tests based on the Jetty11 work from #309 * Fix version * Fix message * Test casing * Update README.md * Make client optional * Add Jetty12 bundle to pom * Store the WebSocket container reference and set it on the shared servlet context once available This removes the need to set setCrossContextDispatchSupported, as the WS container is available on the proper servlet context itself * Update example to no longer use the root context. Removed other WebSocket example, as this is no longer needed with the new approach. Updated documentation. * Add servlet based example again, as it shows another example of how to register a WebSocket endpoint that abides to the servlet context it's registered to. * Rename class * Comment * Comments * Remove classloader code as it now also works without * Rename test class to EE10 * Small changes to README.md
Created PR: #312 |
@enapps-enorman i just noticed that in the Jetty11 bundle, there is this import: https://github.com/apache/felix-dev/blob/master/http/jetty/src/main/java/org/apache/felix/http/jetty/internal/JettyService.java#L56C1-L56C73 I think this could cause issues when the Jetty websocket bundle is not loaded. Does this need adjusting before the release? Created #313 for it. |
I was trying to make this work with jetty 11.x before I saw there was similar work for the jetty12 branch. So this is a slightly different approach than what was merged for #298
The changes I'm proposing here will initialize the websocket support in the main ServletContextHandler
This was trying to follow the guidance from the jetty documentation at: https://eclipse.dev/jetty/documentation/jetty-11/programming-guide/index.html#pg-server-websocket
How this changes the http.jetty bundle:
No new jetty classes embedded in the http.jetty bundle. The websocket related classes remain optional and would be expected to be supplied by deploying the appropriate jetty bundles.
The following new entries are now in the Import-Package instruction to declare the new optional Import-Package values:
org.eclipse.jetty.websocket.server.config;version="[11.0,12)";resolution:=optional
org.eclipse.jetty.websocket.jakarta.server.config;version="[11.0,12)";resolution:=optional
These two new configuration properties are available in the JettyConfig that specify whether to enable the two possible websocket implementations
org.apache.felix.jakarta.ee9.websocket.enable
when set to true, the JakartaWebSocketServletContainerInitializer is called to setup the ServerContainer for the web application context. This also requires that the org.eclipse.jetty.websocket:websocket-jakarta-server bundle is also deployed and active. If that bundle is missing a warning is logged and the websocket initialization is skipped.
org.apache.felix.jetty.websocket.enable
when set to true, the JettyWebSocketServletContainerInitializer is called to setup the JettyWebSocketServerContainer for this web application context. This also requires that the org.eclipse.jetty.websocket:websocket-jetty-server bundle is also deployed and active. If that bundle is missing a warning is logged and the websocket initialization is skipped.
Then one way to declare a custom websocket from a servlet component in a bundle would be to override the servlet init method and do something like below: