From 2a4d672fc1ccf82a57ee6a98fe0a718dcdf3b104 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 6 Oct 2020 18:28:30 +1100 Subject: [PATCH 1/2] Issue #5378 - improve testing for WebSocketUpgradeFilter Signed-off-by: Lachlan Roberts --- .../tests/JettyWebSocketFilterTest.java | 93 ++++++++++++++++++- 1 file changed, 89 insertions(+), 4 deletions(-) diff --git a/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/JettyWebSocketFilterTest.java b/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/JettyWebSocketFilterTest.java index e1786c196b8c..c0be53db2630 100644 --- a/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/JettyWebSocketFilterTest.java +++ b/jetty-websocket/websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/JettyWebSocketFilterTest.java @@ -21,19 +21,24 @@ import java.net.URI; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; +import javax.servlet.http.HttpServlet; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.servlet.FilterHolder; import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.eclipse.jetty.util.component.AbstractLifeCycle; import org.eclipse.jetty.websocket.api.Session; import org.eclipse.jetty.websocket.client.WebSocketClient; import org.eclipse.jetty.websocket.server.JettyWebSocketServerContainer; import org.eclipse.jetty.websocket.server.config.JettyWebSocketServletContainerInitializer; +import org.eclipse.jetty.websocket.util.server.WebSocketUpgradeFilter; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -45,8 +50,17 @@ public class JettyWebSocketFilterTest private WebSocketClient client; private ServletContextHandler contextHandler; - @BeforeEach - public void start() throws Exception + public void start(JettyWebSocketServletContainerInitializer.Configurator configurator) throws Exception + { + start(configurator, null); + } + + public void start(ServletHolder servletHolder) throws Exception + { + start(null, servletHolder); + } + + public void start(JettyWebSocketServletContainerInitializer.Configurator configurator, ServletHolder servletHolder) throws Exception { server = new Server(); connector = new ServerConnector(server); @@ -54,9 +68,11 @@ public void start() throws Exception contextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS); contextHandler.setContextPath("/"); + if (servletHolder != null) + contextHandler.addServlet(servletHolder, "/"); server.setHandler(contextHandler); - JettyWebSocketServletContainerInitializer.configure(contextHandler, null); + JettyWebSocketServletContainerInitializer.configure(contextHandler, configurator); server.start(); client = new WebSocketClient(); @@ -70,9 +86,37 @@ public void stop() throws Exception server.stop(); } + @Test + public void testWebSocketUpgradeFilter() throws Exception + { + start((context, container) -> container.addMapping("/", EchoSocket.class)); + + // After mapping is added we have an UpgradeFilter. + assertThat(contextHandler.getServletHandler().getFilters().length, is(1)); + FilterHolder filterHolder = contextHandler.getServletHandler().getFilter("WebSocketUpgradeFilter"); + assertNotNull(filterHolder); + assertThat(filterHolder.getState(), is(AbstractLifeCycle.STARTED)); + assertThat(filterHolder.getFilter(), instanceOf(WebSocketUpgradeFilter.class)); + + // Test we can upgrade to websocket and send a message. + URI uri = URI.create("ws://localhost:" + connector.getLocalPort() + "/filterPath"); + EventSocket socket = new EventSocket(); + CompletableFuture connect = client.connect(socket, uri); + try (Session session = connect.get(5, TimeUnit.SECONDS)) + { + session.getRemote().sendString("hello world"); + } + assertTrue(socket.closeLatch.await(10, TimeUnit.SECONDS)); + + String msg = socket.textMessages.poll(); + assertThat(msg, is("hello world")); + } + @Test public void testLazyWebSocketUpgradeFilter() throws Exception { + start(null, null); + // JettyWebSocketServerContainer has already been created. JettyWebSocketServerContainer container = JettyWebSocketServerContainer.getContainer(contextHandler.getServletContext()); assertNotNull(container); @@ -83,6 +127,47 @@ public void testLazyWebSocketUpgradeFilter() throws Exception // After mapping is added we have an UpgradeFilter. container.addMapping("/", EchoSocket.class); assertThat(contextHandler.getServletHandler().getFilters().length, is(1)); + FilterHolder filterHolder = contextHandler.getServletHandler().getFilter("WebSocketUpgradeFilter"); + assertNotNull(filterHolder); + assertThat(filterHolder.getState(), is(AbstractLifeCycle.STARTED)); + assertThat(filterHolder.getFilter(), instanceOf(WebSocketUpgradeFilter.class)); + + // Test we can upgrade to websocket and send a message. + URI uri = URI.create("ws://localhost:" + connector.getLocalPort() + "/filterPath"); + EventSocket socket = new EventSocket(); + CompletableFuture connect = client.connect(socket, uri); + try (Session session = connect.get(5, TimeUnit.SECONDS)) + { + session.getRemote().sendString("hello world"); + } + assertTrue(socket.closeLatch.await(10, TimeUnit.SECONDS)); + + String msg = socket.textMessages.poll(); + assertThat(msg, is("hello world")); + } + + @Test + public void testWebSocketUpgradeFilterWhileStarting() throws Exception + { + start(new ServletHolder(new HttpServlet() + { + @Override + public void init() + { + JettyWebSocketServerContainer container = JettyWebSocketServerContainer.getContainer(getServletContext()); + if (container == null) + throw new IllegalArgumentException("Missing JettyWebSocketServerContainer"); + + container.addMapping("/", EchoSocket.class); + } + })); + + // After mapping is added we have an UpgradeFilter. + assertThat(contextHandler.getServletHandler().getFilters().length, is(1)); + FilterHolder filterHolder = contextHandler.getServletHandler().getFilter("WebSocketUpgradeFilter"); + assertNotNull(filterHolder); + assertThat(filterHolder.getState(), is(AbstractLifeCycle.STARTED)); + assertThat(filterHolder.getFilter(), instanceOf(WebSocketUpgradeFilter.class)); // Test we can upgrade to websocket and send a message. URI uri = URI.create("ws://localhost:" + connector.getLocalPort() + "/filterPath"); From 7003b3c42e06425b6dc07599039b1661811c3e19 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Thu, 8 Oct 2020 08:45:38 +1100 Subject: [PATCH 2/2] Issue #5378 - guard against concurrent requests lazily initializing the filter Signed-off-by: Lachlan Roberts --- .../util/server/WebSocketUpgradeFilter.java | 43 +++++++++++-------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/jetty-websocket/websocket-util-server/src/main/java/org/eclipse/jetty/websocket/util/server/WebSocketUpgradeFilter.java b/jetty-websocket/websocket-util-server/src/main/java/org/eclipse/jetty/websocket/util/server/WebSocketUpgradeFilter.java index 5a64fc85ef07..d756b0bcc182 100644 --- a/jetty-websocket/websocket-util-server/src/main/java/org/eclipse/jetty/websocket/util/server/WebSocketUpgradeFilter.java +++ b/jetty-websocket/websocket-util-server/src/main/java/org/eclipse/jetty/websocket/util/server/WebSocketUpgradeFilter.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.time.Duration; import java.util.EnumSet; +import java.util.Objects; import javax.servlet.DispatcherType; import javax.servlet.Filter; import javax.servlet.FilterChain; @@ -37,6 +38,7 @@ import org.eclipse.jetty.servlet.ServletHandler; import org.eclipse.jetty.util.annotation.ManagedObject; import org.eclipse.jetty.util.component.Dumpable; +import org.eclipse.jetty.util.thread.AutoLock; import org.eclipse.jetty.websocket.core.Configuration; import org.eclipse.jetty.websocket.core.server.WebSocketServerComponents; import org.eclipse.jetty.websocket.util.server.internal.WebSocketMapping; @@ -74,10 +76,12 @@ public class WebSocketUpgradeFilter implements Filter, Dumpable { private static final Logger LOG = LoggerFactory.getLogger(WebSocketUpgradeFilter.class); + private static final AutoLock LOCK = new AutoLock(); private static FilterHolder getFilter(ServletContext servletContext) { - ServletHandler servletHandler = ContextHandler.getContextHandler(servletContext).getChildHandlerByClass(ServletHandler.class); + ContextHandler contextHandler = Objects.requireNonNull(ContextHandler.getContextHandler(servletContext)); + ServletHandler servletHandler = contextHandler.getChildHandlerByClass(ServletHandler.class); for (FilterHolder holder : servletHandler.getFilters()) { @@ -105,22 +109,27 @@ private static FilterHolder getFilter(ServletContext servletContext) */ public static FilterHolder ensureFilter(ServletContext servletContext) { - FilterHolder existingFilter = WebSocketUpgradeFilter.getFilter(servletContext); - if (existingFilter != null) - return existingFilter; - - final String name = "WebSocketUpgradeFilter"; - final String pathSpec = "/*"; - FilterHolder holder = new FilterHolder(new WebSocketUpgradeFilter()); - holder.setName(name); - holder.setInitParameter(MAPPING_ATTRIBUTE_INIT_PARAM, WebSocketMapping.DEFAULT_KEY); - - holder.setAsyncSupported(true); - ServletHandler servletHandler = ContextHandler.getContextHandler(servletContext).getChildHandlerByClass(ServletHandler.class); - servletHandler.addFilterWithMapping(holder, pathSpec, EnumSet.of(DispatcherType.REQUEST)); - if (LOG.isDebugEnabled()) - LOG.debug("Adding {} mapped to {} in {}", holder, pathSpec, servletContext); - return holder; + // Lock in case two concurrent requests are initializing the filter lazily. + try (AutoLock l = LOCK.lock()) + { + FilterHolder existingFilter = WebSocketUpgradeFilter.getFilter(servletContext); + if (existingFilter != null) + return existingFilter; + + final String name = "WebSocketUpgradeFilter"; + final String pathSpec = "/*"; + FilterHolder holder = new FilterHolder(new WebSocketUpgradeFilter()); + holder.setName(name); + holder.setInitParameter(MAPPING_ATTRIBUTE_INIT_PARAM, WebSocketMapping.DEFAULT_KEY); + + holder.setAsyncSupported(true); + ContextHandler contextHandler = Objects.requireNonNull(ContextHandler.getContextHandler(servletContext)); + ServletHandler servletHandler = contextHandler.getChildHandlerByClass(ServletHandler.class); + servletHandler.addFilterWithMapping(holder, pathSpec, EnumSet.of(DispatcherType.REQUEST)); + if (LOG.isDebugEnabled()) + LOG.debug("Adding {} mapped to {} in {}", holder, pathSpec, servletContext); + return holder; + } } public static final String MAPPING_ATTRIBUTE_INIT_PARAM = "org.eclipse.jetty.websocket.util.server.internal.WebSocketMapping.key";