WebSocketClient not removing closed WebSocket Session's from managed beans

[bistvan]

Hi,

when a Websocket Session is closed, but instead of closing the WebSocketClient, a new websocket is opened, the annotated Websocket Handler is kept in the memory.

Please find the attached java example (put a breakpoint at line "System.out.println("Put a breakpoint here")". When it stops the client.toString() will show 100 open sessions, but they are already closed.

JettyClientCloseBug.zip

[joakime]

Can replicate.

Client Sessions are not being removed from WebSocketClient beans properly.

[joakime]

Merged into jetty-9.4.x

[menulis]

Websocket sessions (and their corresponding WebSocketClient threads) terminated due to idle timeout do not seem to be properly destroyed as of 9.4.14.v20181114. Implicitly calling close() on javax.websocket.Session inside Endpoint's onError/onClose or elsewhere does not help (though I assume this should not be necessary as Session.isOpen already reports false).

Is it so that in case of embedded Jetty, as of 9.4.14.v20181114 in order to properly recycle Session and respective threads, it is still necessary to cast WebSocketContainer into Lifecycle and call stop as shown in embedded-jetty-websocket-examples?

Also tested and confirming that casting WebSocketContainer to ClientContainer and calling getClient().stop() on it does the cleanup of threads as expected:

WebSocketContainer container = ContainerProvider.getWebSocketContainer();
Session session =  container.connectToServer(this, new URI("ws://test"));

// ...

session.close();

if (container instanceof ClientContainer) {
    ((ClientContainer) container).getClient().stop();
}

[menulis]

@joakime Is it possible to reopen this bug as per my report one month ago?

[sbordet]

@menulis can you try the latest code on branch jetty-9.4.x, or the staged release at https://oss.sonatype.org/content/repositories/jetty-1451?

[menulis]

Thanks for the reply, @sbordet. I tried 9.4.15.v2019021 and I no longer see the problem of JsrSession object buildup inside the ClientContainer, eventually leading to OOM, which is great. Previously with 9.4.14.v20181114 this was as following:

With respect to the increasing number of threads with every newly established session, this is still the case if sessions are esablished by ContainerProvider.getWebSocketContainer().connectToServer(), that is, for every new session, a new WebSocketContainer instance is created (unless JettyClientContainerProvider.willUseSingleton(true) is called?). The workaround is to either call stop() on (casted) WebSocketClient when closing the session or call ContainerProvider.getWebSocketContainer() only once and use it for all subsequent connectToServer() calls.

Do you have any estimates about when the next release will be available?

[joakime]

jakartaee/websocket#212 - From a purely JSR356 point of view, there is no lifecycle for a JSR356 Client Container. It is supposed to live for the entirety of the JVM.

JettyClientContainerProvider.willUseSingleton(true) is a hack within Jetty to allow each call to ContainerProvider.getWebSocketContainer() to return a single instance, vs how the JSR356 API says it will return a new container with each call.

In regards to stop(), that's a Jetty LifeCycle behavior.
If you have any other Jetty components that are an instance of ContainerLifeCycle then you can simply do thatContainerComponent.addBean(websocketclientinstance); and then that container component will automatically stop the websocket client when that container component stops.

Example:
https://github.com/eclipse/jetty.project/blob/8973f01dc4935339743715898ab9dc9e894da5b5/jetty-websocket/javax-websocket-client-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/CookiesTest.java#L108-L109

[sbordet]

@menulis your usage of WebSocketContainer is wrong: you should not create an instance for every attempt to connect to the server.
You create one instance, and then reuse that multiple times to connect to the server.

Regarding stopping the WebSocketContainer, you can follow what @joakime suggests, or this simpler approach:

WebSocketContainer container = ContainerProvider.getWebSocketContainer();
container.connectToServer(...);

// When done with the container:
LifeCycle.stop(container);

[menulis]

Thanks for clarifications, @sbordet and @joakime, they make a lot of sense and I have already adjusted the code accordingly. Stopping the container upon the shutdown of JVM is in fact a minor problem when compared to the memory leak due to JsrSession object buildup inside the ClientContainer and I am glad this is fixed in the staging release 9.4.15.v2019021. Do you have any estimates of when it will be pushed to the next public release?

[sbordet]

@menulis Jetty 9.4.15 has landed in Maven Central, so it's now available to the public. Thanks for your feedback on this issue!