FELIX-6692 jetty 12.x websockets new approach (based on #309) #310

paulrutter · 2024-04-26T09:41:20Z

Based on the Jetty 11.x websocket work in #309, a new PR that incorporates a similar style for Jetty 12.
It adds two new classifiers:

with-jetty-ee10-websockets: jar with the additional Jetty EE10 websocket classes
with-jakarta-ee10-websockets: jar with the additional Jakarta EE10 websocket classes

The main jar and the light jar remain the same.
Two new configuration options:

org.apache.felix.jakarta.ee10.websocket.enable
org.apache.felix.jetty.ee10.websocket.enable

Needs additional tests/examples though, not a finished PR yet.

This reverts commit 6d95c93.

- 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

Do note that the workaround in FelixJettyWebSocketServlet are still required; the initialization code in the Jetty12 bundle doesn't seem to work

paulrutter · 2024-04-26T13:08:09Z

I tried to adapt the example i had and remove the workarounds that we had before, but these are still required to get it to work. The initialization code in the JettyService does not seem to be enough for Jetty12:

In the example from @enapps-enorman:

    private static final class MyWebSocketInitServlet extends HttpServlet {
        private static final long serialVersionUID = -6893620059263229183L;

        @Override
        public void init(ServletConfig config) throws ServletException {
            super.init(config);

            // The websocket container is attached to the main servlet context, not the per-bundle one.
            //  Lookup the ServletContext for the context path where the websocket server is attached.
            ServletContext servletContext = config.getServletContext();
            ServletContext rootContext = servletContext.getContext("/");

            // Retrieve the JettyWebSocketServerContainer.
            JettyWebSocketServerContainer container = JettyWebSocketServerContainer.getContainer(rootContext);
            assertNotNull(container);
            container.addMapping("/mywebsocket1", (upgradeRequest, upgradeResponse) -> new MyServerWebSocket());
        }
    }

ServletContext rootContext = servletContext.getContext("/"); returns null, when i tried it in the whiteboard example bundle.
Any ideas?

cziegeler · 2024-04-29T05:17:45Z

As we use the same Apache Felix code base for both, maybe this is something that changed in Jetty 12? Although that sounds a little bit wired

paulrutter · 2024-04-29T05:25:18Z

I do see jetty/jetty.project#9925, but that has already been fixed. Maybe i can debug the jetty code a bit to see where it fails.

paulrutter · 2024-04-29T06:53:14Z

I found out that 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 is the culprit, and setting context.setCrossContextDispatchSupported(true); in JettyService will solve it.

I can get the example to work now 👍 One downside is that the current servlet context is irrelevant to the websocket being registered.

container.addMapping("/websocket", (upgradeRequest, upgradeResponse) -> new TestWebSocket());

Registers the websocket on ws://localhost:8080/websocket, while the init servlet is registered on ws://localhost:8080/filtersample/websocket/*.

Not sure if this is a big problem though.
I will commit my changes in the code so we have two examples:

my example, which is not based on the initialization code in JettyService. It it's then properly registered to ws://localhost:8080/filtersample/websocket/example
the example from @enapps-enorman, which registeres the websocket to the main servlet context ("/")

Also, i will try to incorporate the same units tests.

paulrutter · 2024-04-29T07:12:37Z

## [class org.apache.felix.http.samples.whiteboard.TestWebsocketServletBundleContext$TestWebSocket] Opened session: WebSocketSession[to=PT30S,WebSocketCoreSession@7946968{SERVER,WebSocketSessionState@7eddbcb3{CONNECTED,i=NO-OP,o=NO-OP,c=null},[ws://localhost:8080/filtersample/websocket1/example,null,false.[]],af=true,i/o=4096/4096,fs=65536}->JettyWebSocketFrameHandler@4e6fb6d0[org.apache.felix.http.samples.whiteboard.TestWebsocketServletBundleContext$TestWebSocket],JettyWebSocketFrameHandler@4e6fb6d0[org.apache.felix.http.samples.whiteboard.TestWebsocketServletBundleContext$TestWebSocket]]
## [class org.apache.felix.http.samples.whiteboard.TestWebsocketServletBundleContext$TestWebSocket] Received message: abc
## [class org.apache.felix.http.samples.whiteboard.TestWebSocketServletMainContext$TestWebSocket] Opened session: WebSocketSession[to=PT30S,WebSocketCoreSession@691aa27d{SERVER,WebSocketSessionState@73f6ef48{CONNECTED,i=NO-OP,o=NO-OP,c=null},[ws://localhost:8080/websocket/example,null,false.[]],af=true,i/o=4096/4096,fs=65536}->JettyWebSocketFrameHandler@11f83f04[org.apache.felix.http.samples.whiteboard.TestWebSocketServletMainContext$TestWebSocket],JettyWebSocketFrameHandler@11f83f04[org.apache.felix.http.samples.whiteboard.TestWebSocketServletMainContext$TestWebSocket]]
## [class org.apache.felix.http.samples.whiteboard.TestWebsocketServletBundleContext$TestWebSocket] Error on session: WebSocketSession[to=PT30S,WebSocketCoreSession@7946968{SERVER,WebSocketSessionState@7eddbcb3{CLOSED,i=NO-OP,o=NO-OP,c={1001=SHUTDOWN,Connection Idle Timeout}},[ws://localhost:8080/filtersample/websocket1/example,null,false.[]],af=true,i/o=4096/4096,fs=65536}->JettyWebSocketFrameHandler@4e6fb6d0[org.apache.felix.http.samples.whiteboard.TestWebsocketServletBundleContext$TestWebSocket],JettyWebSocketFrameHandler@4e6fb6d0[org.apache.felix.http.samples.whiteboard.TestWebsocketServletBundleContext$TestWebSocket]] - org.eclipse.jetty.websocket.api.exceptions.WebSocketTimeoutException: Connection Idle Timeout
## [class org.apache.felix.http.samples.whiteboard.TestWebsocketServletBundleContext$TestWebSocket] Closed session: WebSocketSession[to=PT30S,WebSocketCoreSession@7946968{SERVER,WebSocketSessionState@7eddbcb3{CLOSED,i=NO-OP,o=NO-OP,c={1001=SHUTDOWN,Connection Idle Timeout}},[ws://localhost:8080/filtersample/websocket1/example,null,false.[]],af=true,i/o=4096/4096,fs=65536}->JettyWebSocketFrameHandler@4e6fb6d0[org.apache.felix.http.samples.whiteboard.TestWebsocketServletBundleContext$TestWebSocket],JettyWebSocketFrameHandler@4e6fb6d0[org.apache.felix.http.samples.whiteboard.TestWebsocketServletBundleContext$TestWebSocket]] - 1001 - Connection Idle Timeout
## [class org.apache.felix.http.samples.whiteboard.TestWebSocketServletMainContext$TestWebSocket] Received message: abc

Both examples work, i will push the changes and move on to the unit tests.

…n 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 -

…ew-approach # Conflicts: # http/jetty12/src/main/java/org/apache/felix/http/jetty/internal/JettyService.java

paulrutter · 2024-04-29T08:05:38Z

@enapps-enorman @cziegeler I took the unit tests from #309 and adjusted these to run on Jetty12.
All succeed now.

I think we have a solid solution in place now. Let me know what you think.

http/jetty12/src/main/java/org/apache/felix/http/jetty/internal/JettyService.java

…let context once available This removes the need to set setCrossContextDispatchSupported, as the WS container is available on the proper servlet context itself

Removed other WebSocket example, as this is no longer needed with the new approach. Updated documentation.

…o register a WebSocket endpoint that abides to the servlet context it's registered to.

paulrutter · 2024-04-30T10:28:41Z

I kept both examples in the whiteboard bundle, as both have value:

https://github.com/apache/felix-dev/pull/310/files#diff-a493047a289b4bd7db717df5c308a98988f0417824efa8819cf0e415a2dc750e is the one that uses the WS container to register the mapping on.
https://github.com/apache/felix-dev/pull/310/files#diff-53fea368c43e981c1e37100c78de7e8b2087eb9aa51ceedefdddc6138ae88ca2 is the example that extends JettyWebSocketServlet, but requires a workaround (see base class).

For my own project, the first approach wouldn't work, as we register multiple instances of the same servlet class which would then overwrite the WS mapping on the WebSocket container. Using the JettyWebSocketServlet approach, this does not happen, as JettyWebSocketServlet is used to upgrade a regular HttpServletRequest to a WebSocket request.

Rename org.apache.felix.jetty.websocket.enable to org.apache.felix.jetty.ee9.websocket.enable Incorporate the changes for using maybeStoreWebSocketContainerAttributes

enapps-enorman

This looks good to me.

http/jetty12/src/test/java/org/apache/felix/http/jetty/it/JettySpecificWebsocketIT.java

paulrutter · 2024-05-01T05:50:58Z

@cziegeler can you check out this PR and #309? Both are now aligned to use the same approach.
Let me know if you agree and if this is good to merge for the next HTTP Jetty/Jetty12 release.

If you're ok with having the two additional classifiers for Jetty12, could these also be published to the maven repo?
Or would you rather just have one classifier containing both jakarta and jetty websocket classes?

* FELIX-6692 Add Jetty WebSocket support for jetty 11.x * FELIX-6692 rename jakarta websocket enable config for future expansion * FELIX-6692 add paxexam integration tests to verify the functionality * FELIX-6692 cleanup * FELIX-6692 merge changes from PR #310 Rename org.apache.felix.jetty.websocket.enable to org.apache.felix.jetty.ee9.websocket.enable Incorporate the changes for using maybeStoreWebSocketContainerAttributes * FELIX-6692 renamed for consistency

cziegeler · 2024-05-01T07:23:34Z

@paulrutter The additional classifiers look good to me, they should be pushed automatically to maven central when we do the release

paulrutter added 2 commits April 26, 2024 08:34

Revert "Add jetty websocket support to Jetty12 (apache#298)"

8ad2e49

This reverts commit 6d95c93.

FELIX-6692 Add Jetty WebSocket support for jetty 12.x

6f9d9da

- 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

paulrutter mentioned this pull request Apr 26, 2024

FELIX-6692 Add Jetty WebSocket support for jetty 11.x #309

Merged

Working example base on previous code.

eb0d888

Do note that the workaround in FelixJettyWebSocketServlet are still required; the initialization code in the Jetty12 bundle doesn't seem to work

paulrutter added 3 commits April 29, 2024 09:14

Merge branch 'master' into feature/FELIX-6692-Jetty-12.x-websockets-n…

fa277f9

…ew-approach # Conflicts: # http/jetty12/src/main/java/org/apache/felix/http/jetty/internal/JettyService.java

Added paxweb unit tests based on the Jetty11 work from apache#309

2e39c11

paulrutter added 6 commits April 29, 2024 10:10

Fix version

f11be31

Fix message

7acaefa

Test casing

ba283d6

Update README.md

541c054

Make client optional

e5be78e

Add Jetty12 bundle to pom

434fb37

enapps-enorman reviewed Apr 30, 2024

View reviewed changes

http/jetty12/src/main/java/org/apache/felix/http/jetty/internal/JettyService.java Outdated Show resolved Hide resolved

paulrutter added 5 commits April 30, 2024 10:37

Store the WebSocket container reference and set it on the shared serv…

38b99d7

…let 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.

3420347

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 t…

37bb183

…o register a WebSocket endpoint that abides to the servlet context it's registered to.

Rename class

9382f67

Comment

04f8664

paulrutter added 2 commits April 30, 2024 12:30

Comments

85ae1d1

Remove classloader code as it now also works without

7c55b10

enapps-enorman approved these changes May 1, 2024

View reviewed changes

http/jetty12/src/test/java/org/apache/felix/http/jetty/it/JettySpecificWebsocketIT.java Outdated Show resolved Hide resolved

Rename test class to EE10

bd2cb10

Small changes to README.md

dcf6ae6

cziegeler approved these changes May 1, 2024

View reviewed changes

cziegeler merged commit 0444ac4 into apache:master May 1, 2024

paulrutter deleted the feature/FELIX-6692-Jetty-12.x-websockets-new-approach branch May 1, 2024 07:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FELIX-6692 jetty 12.x websockets new approach (based on #309) #310

FELIX-6692 jetty 12.x websockets new approach (based on #309) #310

paulrutter commented Apr 26, 2024 •

edited

paulrutter commented Apr 26, 2024 •

edited

cziegeler commented Apr 29, 2024

paulrutter commented Apr 29, 2024 •

edited

paulrutter commented Apr 29, 2024 •

edited

paulrutter commented Apr 29, 2024

paulrutter commented Apr 29, 2024

paulrutter commented Apr 30, 2024

enapps-enorman left a comment

paulrutter commented May 1, 2024 •

edited

cziegeler commented May 1, 2024

FELIX-6692 jetty 12.x websockets new approach (based on #309) #310

FELIX-6692 jetty 12.x websockets new approach (based on #309) #310

Conversation

paulrutter commented Apr 26, 2024 • edited

paulrutter commented Apr 26, 2024 • edited

cziegeler commented Apr 29, 2024

paulrutter commented Apr 29, 2024 • edited

paulrutter commented Apr 29, 2024 • edited

paulrutter commented Apr 29, 2024

paulrutter commented Apr 29, 2024

paulrutter commented Apr 30, 2024

enapps-enorman left a comment

Choose a reason for hiding this comment

paulrutter commented May 1, 2024 • edited

cziegeler commented May 1, 2024

paulrutter commented Apr 26, 2024 •

edited

paulrutter commented Apr 26, 2024 •

edited

paulrutter commented Apr 29, 2024 •

edited

paulrutter commented Apr 29, 2024 •

edited

paulrutter commented May 1, 2024 •

edited