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 all 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
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.websocket.core.client;
requires org.eclipse.jetty.websocket.javax.common;
requires transitive org.eclipse.jetty.client;
Expand Down
Expand Up @@ -18,7 +18,6 @@

import org.eclipse.jetty.client.HttpClient;
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 @@ -64,15 +63,8 @@ protected WebSocketContainer getContainer()
public WebSocketContainer getContainer(HttpClient httpClient)
{
JavaxWebSocketClientContainer clientContainer = new JavaxWebSocketClientContainer(httpClient);
registerShutdown(clientContainer);
// See: https://github.com/eclipse-ee4j/websocket-api/issues/212
LifeCycle.start(clientContainer);
return clientContainer;
}

// See: https://github.com/eclipse-ee4j/websocket-api/issues/212
private void registerShutdown(JavaxWebSocketClientContainer container)
{
// Register as JVM runtime shutdown hook.
ShutdownThread.register(container);
LifeCycle.start(container);
}
}
@@ -0,0 +1,73 @@
//
// ========================================================================
// Copyright (c) 1995-2021 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;

/**
* <p>This manages the LifeCycle of {@link javax.websocket.WebSocketContainer} instances that are created with
* {@link javax.websocket.ContainerProvider}, if this code is being run from another ServletContainer, or if run inside a
* Jetty Server with the WebSocket client classes provided by the webapp.</p>
*
* <p>This mechanism will not work if run with embedded Jetty or if the WebSocket client classes are provided by the server.
* In this case then the client {@link javax.websocket.WebSocketContainer} will register itself to be automatically shutdown
* with the Jetty {@code ContextHandler}.</p>
*/
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);
}

@Override
public String toString()
{
return String.format("%s@%x{%s, size=%s}", getClass().getSimpleName(), hashCode(), getState(), getBeans().size());
}
}
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,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<>();
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;
Expand Down Expand Up @@ -261,4 +277,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 (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
Expand Up @@ -28,7 +28,6 @@

import org.eclipse.jetty.servlet.FilterHolder;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.servlet.listener.ContainerInitializer;
import org.eclipse.jetty.util.TypeUtil;
import org.eclipse.jetty.util.thread.ThreadClassLoaderScope;
import org.eclipse.jetty.websocket.core.WebSocketComponents;
Expand All @@ -52,6 +51,18 @@ public class JavaxWebSocketServletContainerInitializer implements ServletContain
public static final String HTTPCLIENT_ATTRIBUTE = "org.eclipse.jetty.websocket.javax.HttpClient";
private static final Logger LOG = LoggerFactory.getLogger(JavaxWebSocketServletContainerInitializer.class);

private final Configurator configurator;

public JavaxWebSocketServletContainerInitializer()
{
this(null);
}

public JavaxWebSocketServletContainerInitializer(Configurator configurator)
{
this.configurator = configurator;
}

/**
* Test a ServletContext for {@code init-param} or {@code attribute} at {@code keyName} for
* true or false setting that determines if the specified feature is enabled (or not).
Expand Down Expand Up @@ -96,28 +107,7 @@ public static void configure(ServletContextHandler context, Configurator configu
{
if (!context.isStopped())
throw new IllegalStateException("configure should be called before starting");

// In this embedded-jetty usage, allow ServletContext.addListener() to
// add other ServletContextListeners (such as the ContextDestroyListener) after
// the initialization phase is over. (important for this SCI to function)
context.getServletContext().setExtendedListenerTypes(true);

context.addEventListener(ContainerInitializer.asContextListener(new JavaxWebSocketServletContainerInitializer())
.afterStartup((servletContext) ->
{
JavaxWebSocketServerContainer serverContainer = JavaxWebSocketServerContainer.getContainer(servletContext);
if (configurator != null)
{
try
{
configurator.accept(servletContext, serverContainer);
}
catch (DeploymentException e)
{
throw new RuntimeException("Failed to deploy WebSocket Endpoint", e);
}
}
}));
context.addServletContainerInitializer(new JavaxWebSocketServletContainerInitializer(configurator));
lachlan-roberts marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -270,6 +260,19 @@ public void onStartup(Set<Class<?>> c, ServletContext context) throws ServletExc
}
}
}

// Call the configurator after startup.
if (configurator != null)
{
try
{
configurator.accept(context, container);
}
catch (DeploymentException e)
{
throw new RuntimeException("Failed to deploy WebSocket Endpoint", e);
}
}
}

@SuppressWarnings("unchecked")
Expand Down
Expand Up @@ -296,4 +296,16 @@ protected void doStart() throws Exception
deferredEndpointConfigs.clear();
}
}

@Override
protected void doClientStart()
{
// Do nothing.
}

@Override
protected void doClientStop()
{
// Do nothing.
}
}