CompressionPools are not shared between multiple contexts for 9.4 WebSocket

[lachlan-roberts]

Target Jetty version(s)
9.4.x

Enhancement Description
There should be only one instance of InflaterPool and DeflaterPool kept on the server and shared with all contexts.

[lachlan-roberts]

Merged PR #7079.

[tresf]

@lachlan-roberts

This change now appears to prevent configuration from being initialized when calling WebSocketUpgradeFilter.configure(context);, potentially causing NPE when calling something like webSocketUpgradeFilter.addMapping(...). A workaround is to use the well-deprecated configureContext(...) which will create the configuration automatically. Is this intended? It forces switching 9.4.x projects to use a deprecated API when one wasn't needed prior. From the surface, this seems like a breaking API change. I'm happy to open a dedicated bug report, but I'd rather not if it's a usage issue. 🍻

Raw error:

java.lang.NullPointerException: Cannot invoke "org.eclipse.jetty.websocket.server.NativeWebSocketConfiguration.addMapping(org.eclipse.jetty.http.pathmap.PathSpec, org.eclipse.jetty.websocket.servlet.WebSocketCreator)" because "this.configuration" is null
  at org.eclipse.jetty.websocket.server.WebSocketUpgradeFilter.addMapping(WebSocketUpgradeFilter.java:157)

[joakime]

@tresf is this embedded Jetty or standalone?

If embedded, how are you initializing websocket?

It should look like this ...

ServletContextHandler servletContextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS);
// various things initializing the ServletContextHandler
WebSocketUpgradeFilter.configure(servletContextHandler);

Note that you can use WebSocketUpgradeFilter and configure it when you setup the ServletContextHandler, like this ...

https://github.com/eclipse/jetty.project/blob/4a0c91c0be53805e3fcffdcdcc9587d5301863db/jetty-websocket/jetty-websocket-tests/src/test/java/org/eclipse/jetty/websocket/tests/PermessageDeflateBufferTest.java#L84-L93

This lambda configures the WebSocketUpgradeFilter during the Servlet initialization phase, which is the only valid phase to initialize and configure websocket mappings.

[lachlan-roberts]

@tresf I see from your stacktrace the error is from PrintSocketServer.

It doesn't make much sense to be configuring the mappings before starting as the mappings are cleared on restart, so this change was intentional as without it other use cases are broken. I see that this change does break your usage of WebSocketUpgradeFilter.addMapping(), I think this method should probably be deprecated as well.

The preferred way to do this is with the NativeWebSocketServletContainerInitializer like Joakim said. Your websocket setup code in PrintSocketServer should now become this:

// Handle WebSocket connections
WebSocketUpgradeFilter filter = WebSocketUpgradeFilter.configure(context);
NativeWebSocketServletContainerInitializer.configure(context, (context, container) ->
{
    container.addMapping(new ServletPathSpec("/"), (req, resp) -> new PrintSocketClient(server));
    container.getPolicy().setMaxTextMessageSize(MAX_MESSAGE_SIZE);
});

I think you would have same logic as before by using the deprecated configureContext(...) method, but this is not recommended and is not supported in newer versions of Jetty (10+).

[tresf]

@lachlan-roberts, @joakime much thanks, minor changes (commented) all working now. 🍻

// Handle WebSocket connections
WebSocketUpgradeFilter.configure(context); // NOTE: return object no longer required
NativeWebSocketServletContainerInitializer.configure(context, (ctx, container) -> { // NOTE: context is already declared
   container.addMapping(new ServletPathSpec("/"), (req, resp) -> new PrintSocketClient(server));
   container.getPolicy().setMaxTextMessageSize(MAX_MESSAGE_SIZE);
});

[joakime]

I went ahead and opened Issue #7771 about this, and updated the javadoc in PR #7772 to clarify this behavior in javadoc a bit more.