Issue #5832 - shutdown Javax WSClientContainer with webapp if possible. #5840

lachlan-roberts · 2020-12-23T00:49:18Z

Issue #5832

If ContainerProvider.getWebSocketContainer() is used within a webapp, we will attach its lifeCycle to the ContextHandler so that it shuts down with the webapp and not in the ShutdownThread.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java

...ain/java/org/eclipse/jetty/websocket/javax/client/JavaxWebSocketClientContainerProvider.java

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java

...et-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/ClientInWebappTest.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java

gregw · 2020-12-30T13:21:30Z

So here is a better idea based on the javax approach that might also work on tomcat deployments:

in the same jar as the client container add a ServletContainerInitializer. This will be discovered only if the client is deployed in a context.
The SCI registers a ServletContextListener so that it can get a stop event - even if no clients are yet started.
The SCI can also set the ServletContext on a static
When a client is started, it can look for the static on the SCI and if set it can add itself as an attribute to the ServletContext (perhaps with a list for multiple clients???)
When the ServletContextListener destoy get's called, it looks for the attribute to find the clients and stops them.

My only concern is the static and how to make that not be nasty if we run embedded or with the jar from the server classpath ??? But I'm sure you'll work it out :)

lachlan-roberts · 2021-01-04T06:32:10Z

@gregw This would require a dependency on javax.servlet for the websocket-javax-client.

And I'm not sure how this would work with multiple webapps using websocket client containers. We couldn't just use a simple static because we would have multiple instances of the ServletContainerInitializer tied to the life cycles of the different webapps. Then we also need to know if the ServletContainerInitializer is not discovered so we can register with the shutdown thread instead.

gregw · 2021-01-04T07:57:14Z

@lachlan-roberts it can be a provided or optional dependency. If you don't run in a servlet container, then nothing will find the SCI and nothing will load the classes that have the dependency.

Multiple webapps are not a problem, as you have multiple classloaders and thus multiple statics.... unless it is loaded from the system classloader. So maybe a static map of ServletContext to client container.. which would often only have a single entry... unless something strange is in the classloading hierarchy.

Note that this is worse than an exception when stopping the maven plugin. Currently I think we are leaking an entire classloader of the webapp over stop/start cycles because the started threads will hold them in memory. So we have to fix this.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

…utility Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw · 2021-01-12T09:47:14Z

@lachlan-roberts is this ready for re-review yet?

lachlan-roberts · 2021-01-12T11:05:48Z

@gregw yes its ready for review.

jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java

...ent/src/main/java/org/eclipse/jetty/websocket/javax/client/JavaxWebSocketClientShutdown.java

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java

...g/eclipse/jetty/websocket/javax/server/config/JavaxWebSocketServletContainerInitializer.java

...ava/org/eclipse/jetty/websocket/server/config/JettyWebSocketServletContainerInitializer.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

…5832-WebSocketShutdownThread

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

janbartel

I'd like to take a look at the use-case for the ServletContextHandler.addServletContainerInitializer methods, and consider the wider picture of how this works with those that are discovered by the AnnotationConfiguration: we should be aiming for more code reuse rather than 2 separate mechanisms.

jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletContextHandler.java

.../src/main/java/org/eclipse/jetty/websocket/javax/client/JavaxWebSocketShutdownContainer.java

...g/eclipse/jetty/websocket/javax/server/config/JavaxWebSocketServletContainerInitializer.java

...ava/org/eclipse/jetty/websocket/server/config/JettyWebSocketServletContainerInitializer.java

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

sbordet · 2021-01-26T08:49:25Z

.../src/main/java/org/eclipse/jetty/websocket/javax/client/JavaxWebSocketShutdownContainer.java

+            LOG.debug("contextDestroyed({}) {}", sce, this);
+
+        LifeCycle.stop(this);
+        removeBeans();


Now that you have implemented doClientStop() I don't think this is necessary.

sbordet · 2021-01-26T08:51:13Z

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java

@@ -95,6 +112,11 @@ public JavaxWebSocketClientContainer(WebSocketComponents components, Function<We
        this.frameHandlerFactory = new JavaxWebSocketClientFrameHandlerFactory(this);
    }

+    public void allowShutdownWithContextHandler(boolean allowShutdownWithContextHandler)


This needs to be a proper setter method, starting with set and the getter should be there too, and used in the code.

sbordet · 2021-01-26T08:55:56Z

...src/test/java/org/eclipse/jetty/websocket/javax/tests/JavaxClientShutdownWithServerTest.java

+        server.stop();
+        assertThat(clientContainer.isRunning(), is(false));
+        assertThat(server.getContainedBeans(WebSocketContainer.class), empty());
+    }


I would add two tests that prove the behavior in case of multiple web applications, since we want to make sure that the static field SHUTDOWN_CONTAINER is not overwritten by other instances.

One test should have the classes provided by the server, and one by the web applications (both).

joakime · 2021-02-11T13:37:11Z

This PR needs copyright header updates.

[INFO] --- license-maven-plugin:3.0:check (check-java-headers) @ jetty-project ---

[INFO] Checking licenses...
[WARNING] Missing header in: PR-5840/jetty-websocket/websocket-javax-tests/src/test/java/org/eclipse/jetty/websocket/javax/tests/JavaxClientShutdownWithServerTest.java
[WARNING] Missing header in: PR-5840/jetty-websocket/websocket-javax-client/src/main/java/org/eclipse/jetty/websocket/javax/client/JavaxWebSocketShutdownContainer.java

…5832-WebSocketShutdownThread Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

gregw · 2021-02-22T11:03:01Z

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java

+            return;
+        }
+
+        ShutdownThread.register(this);


comment here

gregw · 2021-02-22T11:04:51Z

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java

@@ -52,6 +58,16 @@
 @ManagedObject("JSR356 Client Container")
 public class JavaxWebSocketClientContainer extends JavaxWebSocketContainer implements javax.websocket.WebSocketContainer
 {
+    private static final Logger LOG = LoggerFactory.getLogger(JavaxWebSocketClientContainer.class);
+    private static final AtomicReference<ContainerLifeCycle> SHUTDOWN_CONTAINER = new AtomicReference<>();


I comment here about the scope of this static with regards to server/context classloaders would be handy

gregw

Perhaps a few more comments, but I think this is fine.

janbartel

I would have expected to see:

context.addServletContainerInitializer(new CdiServletContainerInitializer())

lachlan-roberts added 4 commits December 22, 2020 11:25

Issue #5832 - deregister ShutdownThread for WebSocketClientContainer

97bbec7

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

register ShutdownThread in doStart

374e02c

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Issue #5832 - shutdown WSClientContainer with ContextHandler if possible

25f8c65

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Issue #5832 - use lifeCycleStopping as lifeCycleStopped is never called

dd1d4bc

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested review from gregw and sbordet December 23, 2020 00:49

janbartel requested changes Dec 23, 2020

View reviewed changes

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java Show resolved Hide resolved

sbordet requested changes Dec 23, 2020

View reviewed changes

Issue #5832 - changes from review

3286f9a

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested review from janbartel and sbordet December 28, 2020 05:09

sbordet requested changes Dec 30, 2020

View reviewed changes

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java Outdated Show resolved Hide resolved

gregw reviewed Dec 30, 2020

View reviewed changes

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java Show resolved Hide resolved

janbartel mentioned this pull request Jan 7, 2021

Classloader leaks from ShutdownThread and QueuedThreadPool #5859

Closed

lachlan-roberts added 2 commits January 8, 2021 22:56

Allow shutdown of Javax WS Client Container though a SCI

774dac6

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Issue #5832 - deprecate and remove usage of the ContainerInitializer …

9e19e87

…utility Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested review from sbordet and gregw January 12, 2021 11:05

gregw mentioned this pull request Jan 12, 2021

o.e.j.servlet.listener.ContainerInitializer does not respect servlet spec contract for ServletContainerInitializers #5834

Closed

sbordet requested changes Jan 12, 2021

View reviewed changes

lachlan-roberts added 5 commits January 13, 2021 17:54

Issue #5832 - changes from review

35051df

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

improve testing for JavaxWebSocketClientContainer shutdown

2f61766

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…

29f1859

…5832-WebSocketShutdownThread

Issue #5832 - fix bugs when stopping the JavaxWebSocketClientContainer

a60ecfa

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Issue #5832 - fix bugs when stopping the JavaxWebSocketClientContainer

99aef1a

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested a review from sbordet January 18, 2021 23:49

janbartel requested changes Jan 19, 2021

View reviewed changes

gregw reviewed Jan 19, 2021

View reviewed changes

...in/java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java Outdated Show resolved Hide resolved

lachlan-roberts added 2 commits January 20, 2021 17:00

Rename JavaxWebSocketClientContainer.initialize to setShutdownContainer.

c6c1ccf

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

only allow adding ServletContainerInitializers before starting

ff4f2ef

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested a review from janbartel January 25, 2021 09:56

sbordet requested changes Jan 26, 2021

View reviewed changes

lachlan-roberts added 3 commits February 22, 2021 09:53

Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…

7760d04

…5832-WebSocketShutdownThread Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Revert changes to ServletContainerInitializer

ed86361

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

Issue #5832 - Improve testing for WebSocket client shutdown.

47f24db

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>

lachlan-roberts requested review from sbordet and gregw February 22, 2021 10:39

gregw reviewed Feb 22, 2021

View reviewed changes

gregw approved these changes Feb 22, 2021

View reviewed changes

janbartel requested changes Feb 22, 2021

View reviewed changes

janbartel approved these changes Feb 23, 2021

View reviewed changes

lachlan-roberts merged commit d9eefc9 into jetty-10.0.x Mar 1, 2021

lachlan-roberts deleted the jetty-10.0.x-5832-WebSocketShutdownThread branch March 1, 2021 05:00

lachlan-roberts mentioned this pull request Mar 1, 2021

Ctrl-C after jetty:run produces NoClassDefFoundError #5832

Closed

sbordet mentioned this pull request Feb 2, 2022

Adding InheritedListeners to already-started components can cause IllegalStateException #7514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #5832 - shutdown Javax WSClientContainer with webapp if possible. #5840

Issue #5832 - shutdown Javax WSClientContainer with webapp if possible. #5840

lachlan-roberts commented Dec 23, 2020

gregw commented Dec 30, 2020

lachlan-roberts commented Jan 4, 2021

gregw commented Jan 4, 2021

gregw commented Jan 12, 2021

lachlan-roberts commented Jan 12, 2021

janbartel left a comment

sbordet Jan 26, 2021

sbordet Jan 26, 2021

sbordet Jan 26, 2021

joakime commented Feb 11, 2021

gregw Feb 22, 2021

gregw Feb 22, 2021

gregw left a comment

janbartel left a comment

Issue #5832 - shutdown Javax WSClientContainer with webapp if possible. #5840

Issue #5832 - shutdown Javax WSClientContainer with webapp if possible. #5840

Conversation

lachlan-roberts commented Dec 23, 2020

gregw commented Dec 30, 2020

lachlan-roberts commented Jan 4, 2021

gregw commented Jan 4, 2021

gregw commented Jan 12, 2021

lachlan-roberts commented Jan 12, 2021

janbartel left a comment

Choose a reason for hiding this comment

sbordet Jan 26, 2021

Choose a reason for hiding this comment

sbordet Jan 26, 2021

Choose a reason for hiding this comment

sbordet Jan 26, 2021

Choose a reason for hiding this comment

joakime commented Feb 11, 2021

gregw Feb 22, 2021

Choose a reason for hiding this comment

gregw Feb 22, 2021

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

janbartel left a comment

Choose a reason for hiding this comment