Skip to content
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

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

Merged
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
97bbec7
Issue #5832 - deregister ShutdownThread for WebSocketClientContainer
lachlan-roberts Dec 22, 2020
374e02c
register ShutdownThread in doStart
lachlan-roberts Dec 22, 2020
25f8c65
Issue #5832 - shutdown WSClientContainer with ContextHandler if possible
lachlan-roberts Dec 23, 2020
dd1d4bc
Issue #5832 - use lifeCycleStopping as lifeCycleStopped is never called
lachlan-roberts Dec 23, 2020
3286f9a
Issue #5832 - changes from review
lachlan-roberts Dec 24, 2020
774dac6
Allow shutdown of Javax WS Client Container though a SCI
lachlan-roberts Jan 8, 2021
9e19e87
Issue #5832 - deprecate and remove usage of the ContainerInitializer …
lachlan-roberts Jan 12, 2021
35051df
Issue #5832 - changes from review
lachlan-roberts Jan 13, 2021
2f61766
improve testing for JavaxWebSocketClientContainer shutdown
lachlan-roberts Jan 14, 2021
29f1859
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
lachlan-roberts Jan 18, 2021
a60ecfa
Issue #5832 - fix bugs when stopping the JavaxWebSocketClientContainer
lachlan-roberts Jan 18, 2021
99aef1a
Issue #5832 - fix bugs when stopping the JavaxWebSocketClientContainer
lachlan-roberts Jan 18, 2021
c6c1ccf
Rename JavaxWebSocketClientContainer.initialize to setShutdownContainer.
lachlan-roberts Jan 20, 2021
ff4f2ef
only allow adding ServletContainerInitializers before starting
lachlan-roberts Jan 21, 2021
7760d04
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
lachlan-roberts Feb 21, 2021
ed86361
Revert changes to ServletContainerInitializer
lachlan-roberts Feb 21, 2021
47f24db
Issue #5832 - Improve testing for WebSocket client shutdown.
lachlan-roberts Feb 22, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -696,6 +696,33 @@ else if (handler instanceof ServletHandler)
relinkHandlers();
}

/**
* Utility Method to allow for manual execution of {@link javax.servlet.ServletContainerInitializer} when using Embedded Jetty.
* @param containerInitializer the ServletContainerInitializer to register.
* @see Initializer
*/
public void addServletContainerInitializer(ServletContainerInitializer containerInitializer)
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
{
if (!isStopped())
throw new IllegalStateException("ServletContainerInitializers should be added before starting");

addServletContainerInitializer(containerInitializer, Collections.emptySet());
}

/**
* Utility Method to allow for manual execution of {@link javax.servlet.ServletContainerInitializer} when using Embedded Jetty.
* @param containerInitializer the ServletContainerInitializer to register.
* @param classes the Set of application classes.
* @see Initializer
*/
public void addServletContainerInitializer(ServletContainerInitializer containerInitializer, Set<Class<?>> classes)
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
{
if (!isStopped())
throw new IllegalStateException("ServletContainerInitializers should be added before starting");

addManaged(new Initializer(this, containerInitializer, classes));
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* The DecoratedObjectFactory for use by IoC containers (weld / spring / etc)
*
Expand Down
Expand Up @@ -25,7 +25,9 @@
/**
* Utility Methods for manual execution of {@link javax.servlet.ServletContainerInitializer} when
* using Embedded Jetty.
* @deprecated use {@link org.eclipse.jetty.servlet.ServletContextHandler#addServletContainerInitializer(ServletContainerInitializer)}.
*/
@Deprecated
public final class ContainerInitializer
{
/**
Expand Down
5 changes: 5 additions & 0 deletions jetty-websocket/websocket-javax-client/pom.xml
Expand Up @@ -34,6 +34,11 @@
<artifactId>jetty-client</artifactId>
<version>${project.version}</version>
</dependency>
<dependency>
<groupId>org.eclipse.jetty.toolchain</groupId>
<artifactId>jetty-servlet-api</artifactId>
<optional>true</optional>
</dependency>
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-xml</artifactId>
Expand Down
Expand Up @@ -20,6 +20,8 @@
exports org.eclipse.jetty.websocket.javax.client;
exports org.eclipse.jetty.websocket.javax.client.internal to org.eclipse.jetty.websocket.javax.server;

requires static jetty.servlet.api;
requires org.slf4j;
requires org.eclipse.jetty.client;
requires org.eclipse.jetty.websocket.core.client;
requires org.eclipse.jetty.websocket.javax.common;
Expand Down
Expand Up @@ -17,7 +17,6 @@
import javax.websocket.WebSocketContainer;

import org.eclipse.jetty.util.component.LifeCycle;
import org.eclipse.jetty.util.thread.ShutdownThread;
import org.eclipse.jetty.websocket.javax.client.internal.JavaxWebSocketClientContainer;

/**
Expand Down Expand Up @@ -59,22 +58,7 @@ protected WebSocketContainer getContainer()
// TODO: do we want to provide a non-standard way to configure to always return the same clientContainer based on a config somewhere? (system.property?)

JavaxWebSocketClientContainer clientContainer = new JavaxWebSocketClientContainer();

// Register as JVM runtime shutdown hook?
ShutdownThread.register(clientContainer);

if (!clientContainer.isStarted())
{
try
{
clientContainer.start();
}
catch (Exception e)
{
throw new RuntimeException("Unable to start Client Container", e);
}
}

LifeCycle.start(clientContainer);
return clientContainer;
}
}
@@ -0,0 +1,58 @@
//
// ========================================================================
// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
//
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
// ========================================================================
//

package org.eclipse.jetty.websocket.javax.client;

import java.util.Set;
import javax.servlet.ServletContainerInitializer;
import javax.servlet.ServletContext;
import javax.servlet.ServletContextEvent;
import javax.servlet.ServletContextListener;
import javax.servlet.ServletException;

import org.eclipse.jetty.util.component.ContainerLifeCycle;
import org.eclipse.jetty.util.component.LifeCycle;
import org.eclipse.jetty.websocket.javax.client.internal.JavaxWebSocketClientContainer;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class JavaxWebSocketShutdownContainer extends ContainerLifeCycle implements ServletContainerInitializer, ServletContextListener
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
{
private static final Logger LOG = LoggerFactory.getLogger(JavaxWebSocketShutdownContainer.class);

@Override
public void onStartup(Set<Class<?>> c, ServletContext ctx) throws ServletException
{
JavaxWebSocketClientContainer.setShutdownContainer(this);
ctx.addListener(this);
}

@Override
public void contextInitialized(ServletContextEvent sce)
{
if (LOG.isDebugEnabled())
LOG.debug("contextInitialized({}) {}", sce, this);
LifeCycle.start(this);
}

@Override
public void contextDestroyed(ServletContextEvent sce)
{
if (LOG.isDebugEnabled())
LOG.debug("contextDestroyed({}) {}", sce, this);

LifeCycle.stop(this);
removeBeans();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

JavaxWebSocketClientContainer.setShutdownContainer(null);
}
}
Expand Up @@ -22,6 +22,7 @@
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Function;
import javax.websocket.ClientEndpoint;
import javax.websocket.ClientEndpointConfig;
Expand All @@ -33,6 +34,9 @@

import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.util.annotation.ManagedObject;
import org.eclipse.jetty.util.component.ContainerLifeCycle;
import org.eclipse.jetty.util.component.LifeCycle;
import org.eclipse.jetty.util.thread.ShutdownThread;
import org.eclipse.jetty.websocket.core.WebSocketComponents;
import org.eclipse.jetty.websocket.core.client.WebSocketCoreClient;
import org.eclipse.jetty.websocket.core.exception.InvalidWebSocketException;
Expand All @@ -43,6 +47,8 @@
import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketExtensionConfig;
import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandler;
import org.eclipse.jetty.websocket.javax.common.JavaxWebSocketFrameHandlerFactory;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Container for Client use of the javax.websocket API.
Expand All @@ -52,9 +58,20 @@
@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<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


public static void setShutdownContainer(ContainerLifeCycle container)
{
SHUTDOWN_CONTAINER.set(container);
if (LOG.isDebugEnabled())
LOG.debug("initialized {} to {}", String.format("%s@%x", SHUTDOWN_CONTAINER.getClass().getSimpleName(), SHUTDOWN_CONTAINER.hashCode()), container);
}

protected WebSocketCoreClient coreClient;
protected Function<WebSocketComponents, WebSocketCoreClient> coreClientFactory;
private final JavaxWebSocketClientFrameHandlerFactory frameHandlerFactory;
private boolean allowShutdownWithContextHandler = true;

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

public void allowShutdownWithContextHandler(boolean allowShutdownWithContextHandler)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
this.allowShutdownWithContextHandler = allowShutdownWithContextHandler;
}

protected HttpClient getHttpClient()
{
return getWebSocketCoreClient().getHttpClient();
Expand Down Expand Up @@ -271,4 +293,123 @@ private ClientEndpointConfig getAnnotatedConfig(Object endpoint) throws Deployme

return new AnnotatedClientEndpointConfig(anno);
}

@Override
protected void doStart() throws Exception
{
doClientStart();
super.doStart();
}

@Override
protected void doStop() throws Exception
{
super.doStop();
doClientStop();
}

protected void doClientStart()
{
if (LOG.isDebugEnabled())
LOG.debug("doClientStart() {}", this);

// If we are running in Jetty register shutdown with the ContextHandler.
if (allowShutdownWithContextHandler && addToContextHandler())
{
if (LOG.isDebugEnabled())
LOG.debug("Shutdown registered with ContextHandler");
return;
}

// If we are running inside a different ServletContainer we can register with the SHUTDOWN_CONTAINER static.
ContainerLifeCycle shutdownContainer = SHUTDOWN_CONTAINER.get();
if (shutdownContainer != null)
{
shutdownContainer.addManaged(this);
if (LOG.isDebugEnabled())
LOG.debug("Shutdown registered with ShutdownContainer {}", shutdownContainer);
return;
}

ShutdownThread.register(this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment here

if (LOG.isDebugEnabled())
LOG.debug("Shutdown registered with ShutdownThread");
}

protected void doClientStop()
{
if (LOG.isDebugEnabled())
LOG.debug("doClientStop() {}", this);

// Remove from context handler if running in Jetty server.
removeFromContextHandler();

// Remove from the Shutdown Container.
ContainerLifeCycle shutdownContainer = SHUTDOWN_CONTAINER.get();
if (shutdownContainer != null && shutdownContainer.contains(this))
{
// Un-manage first as we don't want to call stop again while in STOPPING state.
shutdownContainer.unmanage(this);
shutdownContainer.removeBean(this);
}

// If not running in a server we need to de-register with the shutdown thread.
ShutdownThread.deregister(this);
}

private boolean addToContextHandler()
{
try
{
Object context = getClass().getClassLoader()
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
.loadClass("org.eclipse.jetty.server.handler.ContextHandler")
.getMethod("getCurrentContext")
.invoke(null);

Object contextHandler = context.getClass()
.getMethod("getContextHandler")
.invoke(context);

contextHandler.getClass()
.getMethod("addManaged", LifeCycle.class)
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
.invoke(contextHandler, this);

return true;
}
catch (Throwable throwable)
{
if (LOG.isDebugEnabled())
LOG.debug("error from addToContextHandler() for {}", this, throwable);
return false;
}
}

private void removeFromContextHandler()
{
try
{
Object context = getClass().getClassLoader()
.loadClass("org.eclipse.jetty.server.handler.ContextHandler")
.getMethod("getCurrentContext")
.invoke(null);

Object contextHandler = context.getClass()
.getMethod("getContextHandler")
.invoke(context);

// Un-manage first as we don't want to call stop again while in STOPPING state.
contextHandler.getClass()
.getMethod("unmanage", Object.class)
.invoke(contextHandler, this);

contextHandler.getClass()
.getMethod("removeBean", Object.class)
.invoke(contextHandler, this);
}
catch (Throwable throwable)
{
if (LOG.isDebugEnabled())
LOG.debug("error from removeFromContextHandler() for {}", this, throwable);
}
}
}
@@ -0,0 +1 @@
org.eclipse.jetty.websocket.javax.client.JavaxWebSocketShutdownContainer