From a03a352d79112b8150ba97b1c45805af22f0c44b Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 22 Jun 2020 11:21:46 +1000 Subject: [PATCH 1/2] Issue #1100 - replicate issue where Encoder init and destroy never called Signed-off-by: Lachlan Roberts --- .../jsr356/server/EncoderLifeCycleTest.java | 186 ++++++++++++++++++ 1 file changed, 186 insertions(+) create mode 100644 jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/EncoderLifeCycleTest.java diff --git a/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/EncoderLifeCycleTest.java b/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/EncoderLifeCycleTest.java new file mode 100644 index 000000000000..b607a8d8c04a --- /dev/null +++ b/jetty-websocket/javax-websocket-server-impl/src/test/java/org/eclipse/jetty/websocket/jsr356/server/EncoderLifeCycleTest.java @@ -0,0 +1,186 @@ +// +// ======================================================================== +// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.websocket.jsr356.server; + +import java.net.URI; +import java.util.Collections; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import javax.websocket.ClientEndpointConfig; +import javax.websocket.CloseReason; +import javax.websocket.ContainerProvider; +import javax.websocket.Encoder; +import javax.websocket.Endpoint; +import javax.websocket.EndpointConfig; +import javax.websocket.MessageHandler; +import javax.websocket.Session; +import javax.websocket.WebSocketContainer; +import javax.websocket.server.ServerEndpointConfig; + +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.util.BlockingArrayQueue; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.Logger; +import org.eclipse.jetty.websocket.jsr356.EncoderFactory; +import org.eclipse.jetty.websocket.jsr356.JsrSession; +import org.eclipse.jetty.websocket.jsr356.server.deploy.WebSocketServerContainerInitializer; +import org.eclipse.jetty.websocket.jsr356.server.samples.echo.EchoReturnEndpoint; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class EncoderLifeCycleTest +{ + private static final Logger LOG = Log.getLogger(EncoderLifeCycleTest.class); + private static Server server; + private static URI serverUri; + + @BeforeAll + public static void startServer() throws Exception + { + server = new Server(); + ServerConnector connector = new ServerConnector(server); + server.addConnector(connector); + + ServletContextHandler contextHandler = new ServletContextHandler(); + contextHandler.setContextPath("/"); + server.setHandler(contextHandler); + + WebSocketServerContainerInitializer.configure(contextHandler, ((servletContext, serverContainer) -> + serverContainer.addEndpoint(ServerEndpointConfig.Builder.create(EchoReturnEndpoint.class, "/").build()))); + + // Start Server + server.start(); + serverUri = new URI(String.format("ws://localhost:%d/", connector.getLocalPort())); + } + + public static class StringHolder + { + private final String string; + + public StringHolder(String msg) + { + string = msg; + } + + public String getString() + { + return string; + } + } + + public static class StringHolderSubtype extends StringHolder + { + public StringHolderSubtype(String msg) + { + super(msg + "|subtype"); + } + } + + public static class MyEncoder implements Encoder.Text + { + public CountDownLatch initialized = new CountDownLatch(1); + public CountDownLatch destroyed = new CountDownLatch(1); + + @Override + public void init(EndpointConfig config) + { + initialized.countDown(); + } + + @Override + public String encode(StringHolder message) + { + return message.getString(); + } + + @Override + public void destroy() + { + destroyed.countDown(); + } + } + + public static class TextMessageEndpoint extends Endpoint implements MessageHandler.Whole + { + public BlockingArrayQueue textMessages = new BlockingArrayQueue<>(); + public CountDownLatch openLatch = new CountDownLatch(1); + public CountDownLatch closeLatch = new CountDownLatch(1); + public CloseReason closeReason = null; + + @Override + public void onOpen(Session session, EndpointConfig config) + { + session.addMessageHandler(this); + this.openLatch.countDown(); + } + + @Override + public void onClose(Session session, CloseReason closeReason) + { + this.closeReason = closeReason; + this.closeLatch.countDown(); + } + + @Override + public void onMessage(String message) + { + this.textMessages.add(message); + } + } + + @ParameterizedTest + @ValueSource(classes = {StringHolder.class, StringHolderSubtype.class}) + public void testEncoderLifeCycle(Class clazz) throws Exception + { + WebSocketContainer container = ContainerProvider.getWebSocketContainer(); + TextMessageEndpoint clientEndpoint = new TextMessageEndpoint(); + ClientEndpointConfig clientConfig = ClientEndpointConfig.Builder.create() + .encoders(Collections.singletonList(MyEncoder.class)) + .build(); + + // Send an instance of our StringHolder type. + Session session = container.connectToServer(clientEndpoint, clientConfig, serverUri); + StringHolder data = clazz.getConstructor(String.class).newInstance("test1"); + session.getBasicRemote().sendObject(data); + + // We received the expected echo. + String echoed = clientEndpoint.textMessages.poll(5, TimeUnit.SECONDS); + assertThat("Echoed message", echoed, is(data.getString())); + + // Verify that the encoder has been opened. + EncoderFactory encoderFactory = ((JsrSession)session).getEncoderFactory(); + Object obj = encoderFactory.getEncoderFor(data.getClass()); + assertThat(obj.getClass(), is(MyEncoder.class)); + MyEncoder encoder = (MyEncoder)obj; + assertThat(encoder.initialized.getCount(), is(0L)); + + // Verify the Encoder has not been destroyed, but is destroyed after the session is closed. + assertThat(encoder.destroyed.getCount(), is(1L)); + session.close(); + assertTrue(clientEndpoint.closeLatch.await(5, TimeUnit.SECONDS)); + assertTrue(encoder.destroyed.await(5, TimeUnit.SECONDS)); + } +} From 0db20886d04726983da69e5041f1b264e714cd06 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 22 Jun 2020 11:40:55 +1000 Subject: [PATCH 2/2] Issue #1100 - ensure init and destroy are always called on JSR356 Encoders Signed-off-by: Lachlan Roberts --- .../websocket/jsr356/ClientContainer.java | 2 ++ .../jetty/websocket/jsr356/Configurable.java | 2 ++ .../websocket/jsr356/DecoderFactory.java | 17 +++++++++++ .../websocket/jsr356/EncoderFactory.java | 29 ++++++++++++++----- .../jetty/websocket/jsr356/JsrSession.java | 7 +++++ .../endpoints/AbstractJsrEventDriver.java | 4 +++ 6 files changed, 54 insertions(+), 7 deletions(-) diff --git a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/ClientContainer.java b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/ClientContainer.java index 0196f11b60aa..32ca8337d573 100644 --- a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/ClientContainer.java +++ b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/ClientContainer.java @@ -308,6 +308,8 @@ protected void doStart() throws Exception protected void doStop() throws Exception { ShutdownThread.deregister(this); + this.encoderFactory.destroy(); + this.decoderFactory.destroy(); endpointClientMetadataCache.clear(); super.doStop(); } diff --git a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/Configurable.java b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/Configurable.java index 352e6478fd4c..8e3dd9b20ada 100644 --- a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/Configurable.java +++ b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/Configurable.java @@ -26,4 +26,6 @@ public interface Configurable { void init(EndpointConfig config); + + void destroy(); } diff --git a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/DecoderFactory.java b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/DecoderFactory.java index 58ad6c629dc1..8b61176fdbd0 100644 --- a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/DecoderFactory.java +++ b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/DecoderFactory.java @@ -69,6 +69,12 @@ public void init(EndpointConfig config) { this.decoder.init(config); } + + @Override + public void destroy() + { + this.decoder.destroy(); + } } private static final Logger LOG = Log.getLogger(DecoderFactory.class); @@ -185,6 +191,17 @@ public void init(EndpointConfig config) } } + @Override + public void destroy() + { + for (Wrapper wrapper : activeWrappers.values()) + { + wrapper.decoder.destroy(); + } + + activeWrappers.clear(); + } + public Wrapper newWrapper(DecoderMetadata metadata) { Class decoderClass = metadata.getCoderClass(); diff --git a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/EncoderFactory.java b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/EncoderFactory.java index 27bf324f2aea..1bbf3a7d4834 100644 --- a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/EncoderFactory.java +++ b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/EncoderFactory.java @@ -62,14 +62,21 @@ public void init(EndpointConfig config) { this.encoder.init(config); } + + @Override + public void destroy() + { + this.encoder.destroy(); + } } private static final Logger LOG = Log.getLogger(EncoderFactory.class); private final EncoderMetadataSet metadatas; private final WebSocketContainerScope containerScope; - private EncoderFactory parentFactory; - private Map, Wrapper> activeWrappers; + private final Map, Wrapper> activeWrappers; + private final EncoderFactory parentFactory; + private EndpointConfig endpointConfig; public EncoderFactory(WebSocketContainerScope containerScope, EncoderMetadataSet metadatas) { @@ -153,10 +160,9 @@ public Wrapper getWrapperFor(Class type) @Override public void init(EndpointConfig config) { + this.endpointConfig = config; if (LOG.isDebugEnabled()) - { - LOG.debug("init({})", config); - } + LOG.debug("init({})", endpointConfig); // Instantiate all declared encoders for (EncoderMetadata metadata : metadatas) @@ -164,20 +170,29 @@ public void init(EndpointConfig config) Wrapper wrapper = newWrapper(metadata); activeWrappers.put(metadata.getObjectType(), wrapper); } + } - // Initialize all encoders + @Override + public void destroy() + { for (Wrapper wrapper : activeWrappers.values()) { - wrapper.encoder.init(config); + wrapper.encoder.destroy(); } + + activeWrappers.clear(); } private Wrapper newWrapper(EncoderMetadata metadata) { + if (endpointConfig == null) + throw new IllegalStateException("EndpointConfig not set"); + Class encoderClass = metadata.getCoderClass(); try { Encoder encoder = containerScope.getObjectFactory().createInstance(encoderClass); + encoder.init(endpointConfig); return new Wrapper(encoder, metadata); } catch (Exception e) diff --git a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/JsrSession.java b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/JsrSession.java index 5dc3e71b96c1..50c48eb39e83 100644 --- a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/JsrSession.java +++ b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/JsrSession.java @@ -306,6 +306,13 @@ public void init(EndpointConfig config) decoderFactory.init(config); } + @Override + public void destroy() + { + encoderFactory.destroy(); + decoderFactory.destroy(); + } + @Override public void removeMessageHandler(MessageHandler handler) { diff --git a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/endpoints/AbstractJsrEventDriver.java b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/endpoints/AbstractJsrEventDriver.java index 4f76e8fa8ad6..e3304a4a2c47 100644 --- a/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/endpoints/AbstractJsrEventDriver.java +++ b/jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/endpoints/AbstractJsrEventDriver.java @@ -77,6 +77,10 @@ public final void onClose(CloseInfo close) CloseCode closecode = CloseCodes.getCloseCode(close.getStatusCode()); CloseReason closereason = new CloseReason(closecode, close.getReason()); onClose(closereason); + + // Destroy the JsrSession. + if (jsrsession != null) + jsrsession.destroy(); } protected abstract void onClose(CloseReason closereason);