Merge pull request #5413 from eclipse/jetty-10.0.x-WebSocketUpgradeFi…

…lter simplify the usage of WebSocketUpgradeFilter in jetty 10
jetty · Nov 6, 2020 · b2bfe7e · b2bfe7e
2 parents 44c7ce4 + f857ac9
commit b2bfe7e
Show file tree

Hide file tree

Showing 9 changed files with 63 additions and 49 deletions.
diff --git a/...st/java/org/eclipse/jetty/websocket/core/extensions/PerMessageDeflaterBufferSizeTest.java b/...st/java/org/eclipse/jetty/websocket/core/extensions/PerMessageDeflaterBufferSizeTest.java
@@ -20,6 +20,7 @@
 
 import java.io.IOException;
 import java.net.URI;
+import java.nio.ByteBuffer;
 import java.util.Objects;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.TimeUnit;
@@ -28,10 +29,13 @@
 import org.eclipse.jetty.client.HttpResponse;
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpHeader;
+import org.eclipse.jetty.util.BufferUtil;
+import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.websocket.core.CoreSession;
 import org.eclipse.jetty.websocket.core.ExtensionConfig;
 import org.eclipse.jetty.websocket.core.Frame;
 import org.eclipse.jetty.websocket.core.FrameHandler;
+import org.eclipse.jetty.websocket.core.OpCode;
 import org.eclipse.jetty.websocket.core.TestFrameHandler;
 import org.eclipse.jetty.websocket.core.WebSocketServer;
 import org.eclipse.jetty.websocket.core.client.CoreClientUpgradeRequest;
@@ -291,7 +295,8 @@ public void onHandshakeResponse(HttpRequest request, HttpResponse response)
 
         // We should now only be able to send this message in multiple frames as it exceeds deflate_buffer_size.
         String message = "0123456789";
-        clientHandler.sendText(message);
+        ByteBuffer payload = toBuffer(message, true);
+        clientHandler.getCoreSession().sendFrame(new Frame(OpCode.TEXT, payload), Callback.NOOP, false);
 
         // Verify the frame has been fragmented into multiple parts.
         int numFrames = 0;
@@ -315,4 +320,18 @@ public void onHandshakeResponse(HttpRequest request, HttpResponse response)
         assertNull(serverHandler.getError());
         assertNull(clientHandler.getError());
     }
+
+    public ByteBuffer toBuffer(String string, boolean direct)
+    {
+        ByteBuffer buffer = BufferUtil.allocate(string.length(), direct);
+        BufferUtil.clearToFill(buffer);
+        BufferUtil.put(BufferUtil.toBuffer(string), buffer);
+        BufferUtil.flipToFlush(buffer, 0);
+
+        // Sanity checks.
+        assertThat(buffer.hasArray(), is(!direct));
+        assertThat(BufferUtil.toString(buffer), is(string));
+
+        return buffer;
+    }
 }
diff --git a/jetty-websocket/websocket-javax-tests/src/test/resources/alt-filter-web.xml b/jetty-websocket/websocket-javax-tests/src/test/resources/alt-filter-web.xml
@@ -14,6 +14,10 @@
   <filter>
     <filter-name>wsuf-test</filter-name>
     <filter-class>org.eclipse.jetty.websocket.util.server.WebSocketUpgradeFilter</filter-class>
+    <init-param>
+      <param-name>jetty.websocket.WebSocketMapping</param-name>
+      <param-value>jetty.websocket.defaultMapping</param-value>
+    </init-param>
   </filter>
 
   <filter-mapping>

diff --git a/...erver/src/main/java/org/eclipse/jetty/websocket/server/JettyWebSocketServerContainer.java b/...erver/src/main/java/org/eclipse/jetty/websocket/server/JettyWebSocketServerContainer.java
@@ -91,6 +91,7 @@ public static JettyWebSocketServerContainer ensureContainer(ServletContext servl
 
     private final ServletContextHandler contextHandler;
     private final WebSocketMapping webSocketMapping;
+    private final WebSocketComponents components;
     private final FrameHandlerFactory frameHandlerFactory;
     private final Executor executor;
     private final Configuration.ConfigurationCustomizer customizer = new Configuration.ConfigurationCustomizer();
@@ -102,14 +103,15 @@ public static JettyWebSocketServerContainer ensureContainer(ServletContext servl
      * Main entry point for {@link JettyWebSocketServletContainerInitializer}.
      *
      * @param webSocketMapping the {@link WebSocketMapping} that this container belongs to
-     * @param webSocketComponents the {@link WebSocketComponents} instance to use
+     * @param components the {@link WebSocketComponents} instance to use
      * @param executor the {@link Executor} to use
      */
-    JettyWebSocketServerContainer(ServletContextHandler contextHandler, WebSocketMapping webSocketMapping, WebSocketComponents webSocketComponents, Executor executor)
+    JettyWebSocketServerContainer(ServletContextHandler contextHandler, WebSocketMapping webSocketMapping, WebSocketComponents components, Executor executor)
     {
         this.contextHandler = contextHandler;
         this.webSocketMapping = webSocketMapping;
         this.executor = executor;
+        this.components = components;
 
         // Ensure there is a FrameHandlerFactory
         JettyServerFrameHandlerFactory factory = contextHandler.getBean(JettyServerFrameHandlerFactory.class);
@@ -155,6 +157,11 @@ public void addMapping(String pathSpec, final Class<?> endpointClass)
         });
     }
 
+    public WebSocketComponents getWebSocketComponents()
+    {
+        return components;
+    }
+
     @Override
     public Executor getExecutor()
     {

diff --git a/.../org/eclipse/jetty/websocket/server/config/JettyWebSocketServletContainerInitializer.java b/.../org/eclipse/jetty/websocket/server/config/JettyWebSocketServletContainerInitializer.java
@@ -92,11 +92,9 @@ public static void configure(ServletContextHandler context, Configurator configu
     private static JettyWebSocketServerContainer initialize(ServletContextHandler context)
     {
         WebSocketComponents components = WebSocketServerComponents.ensureWebSocketComponents(context.getServer(), context.getServletContext());
-        WebSocketMapping mapping = WebSocketMapping.ensureMapping(context.getServletContext(), WebSocketMapping.DEFAULT_KEY);
         JettyWebSocketServerContainer container = JettyWebSocketServerContainer.ensureContainer(context.getServletContext());
-
         if (LOG.isDebugEnabled())
-            LOG.debug("configureContext {} {} {}", container, mapping, components);
+            LOG.debug("initialize {} {}", container, components);
 
         return container;
     }
@@ -105,6 +103,8 @@ private static JettyWebSocketServerContainer initialize(ServletContextHandler co
     public void onStartup(Set<Class<?>> c, ServletContext context)
     {
         ServletContextHandler contextHandler = ServletContextHandler.getServletContextHandler(context, "Jetty WebSocket SCI");
-        JettyWebSocketServletContainerInitializer.initialize(contextHandler);
+        JettyWebSocketServerContainer container = JettyWebSocketServletContainerInitializer.initialize(contextHandler);
+        if (LOG.isDebugEnabled())
+            LOG.debug("onStartup {}", container);
     }
 }
diff --git a/...websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/ErrorCloseTest.java b/...websocket-jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/ErrorCloseTest.java
@@ -22,11 +22,9 @@
 import java.net.URI;
 import java.time.Duration;
 import java.util.ArrayList;
-import java.util.EnumSet;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
-import javax.servlet.DispatcherType;
 
 import org.eclipse.jetty.logging.StacklessLogging;
 import org.eclipse.jetty.server.Server;
@@ -39,7 +37,6 @@
 import org.eclipse.jetty.websocket.common.WebSocketSession;
 import org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession;
 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;
@@ -54,22 +51,20 @@
 
 public class ErrorCloseTest
 {
-    private Server server = new Server();
-    private WebSocketClient client = new WebSocketClient();
-    private ThrowingSocket serverSocket = new ThrowingSocket();
-    private CountDownLatch serverCloseListener = new CountDownLatch(1);
-    private ServerConnector connector;
+    private final Server server = new Server();
+    private final WebSocketClient client = new WebSocketClient();
+    private final ThrowingSocket serverSocket = new ThrowingSocket();
+    private final CountDownLatch serverCloseListener = new CountDownLatch(1);
     private URI serverUri;
 
     @BeforeEach
     public void start() throws Exception
     {
-        connector = new ServerConnector(server);
+        ServerConnector connector = new ServerConnector(server);
         server.addConnector(connector);
 
         ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS);
         contextHandler.setContextPath("/");
-        contextHandler.addFilter(WebSocketUpgradeFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
         JettyWebSocketServletContainerInitializer.configure(contextHandler, (context, container) ->
         {
             container.addMapping("/", (req, resp) -> serverSocket);
@@ -140,7 +135,7 @@ public void testOnOpenThrows() throws Exception
         serverSocket.methodsToThrow.add("onOpen");
         EventSocket clientSocket = new EventSocket();
 
-        try (StacklessLogging stacklessLogging = new StacklessLogging(WebSocketCoreSession.class))
+        try (StacklessLogging ignored = new StacklessLogging(WebSocketCoreSession.class))
         {
             client.connect(clientSocket, serverUri).get(5, TimeUnit.SECONDS);
             assertTrue(serverSocket.closeLatch.await(5, TimeUnit.SECONDS));

diff --git a/...jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java b/...jetty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/ClientConnectTest.java
@@ -25,15 +25,13 @@
 import java.net.SocketTimeoutException;
 import java.net.URI;
 import java.time.Duration;
-import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
-import javax.servlet.DispatcherType;
 
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
@@ -49,7 +47,6 @@
 import org.eclipse.jetty.websocket.tests.EchoSocket;
 import org.eclipse.jetty.websocket.tests.GetAuthHeaderEndpoint;
 import org.eclipse.jetty.websocket.tests.SimpleStatusServlet;
-import org.eclipse.jetty.websocket.util.server.WebSocketUpgradeFilter;
 import org.hamcrest.Matcher;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
@@ -73,7 +70,7 @@ public class ClientConnectTest
 {
     private Server server;
     private WebSocketClient client;
-    private CountDownLatch serverLatch = new CountDownLatch(1);
+    private final CountDownLatch serverLatch = new CountDownLatch(1);
 
     @SuppressWarnings("unchecked")
     private <E extends Throwable> E assertExpectedError(ExecutionException e, CloseTrackingEndpoint wsocket, Matcher<Throwable> errorMatcher)
@@ -143,8 +140,6 @@ public void startServer() throws Exception
                 });
             });
 
-        context.addFilter(WebSocketUpgradeFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
-
         context.addServlet(new ServletHolder(new SimpleStatusServlet(404)), "/bogus");
         context.addServlet(new ServletHolder(new SimpleStatusServlet(200)), "/a-okay");
         context.addServlet(new ServletHolder(new InvalidUpgradeServlet()), "/invalid-upgrade/*");

diff --git a/...tty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/WebSocketClientTest.java b/...tty-tests/src/test/java/org/eclipse/jetty/websocket/tests/client/WebSocketClientTest.java
@@ -25,12 +25,10 @@
 import java.time.Duration;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
-import javax.servlet.DispatcherType;
 
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.ServerConnector;
@@ -50,7 +48,6 @@
 import org.eclipse.jetty.websocket.tests.EchoSocket;
 import org.eclipse.jetty.websocket.tests.ParamsEndpoint;
 import org.eclipse.jetty.websocket.tests.util.FutureWriteCallback;
-import org.eclipse.jetty.websocket.util.server.WebSocketUpgradeFilter;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeEach;
@@ -104,10 +101,7 @@ public void startServer() throws Exception
                 configuration.addMapping("/get-params", (req, resp) -> new ParamsEndpoint());
             });
 
-        context.addFilter(WebSocketUpgradeFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
-
         server.setHandler(context);
-
         server.start();
     }
 

diff --git a/...-server/src/main/java/org/eclipse/jetty/websocket/util/server/WebSocketUpgradeFilter.java b/...-server/src/main/java/org/eclipse/jetty/websocket/util/server/WebSocketUpgradeFilter.java
@@ -40,7 +40,6 @@
 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;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -78,34 +77,40 @@ public class WebSocketUpgradeFilter implements Filter, Dumpable
     private static final Logger LOG = LoggerFactory.getLogger(WebSocketUpgradeFilter.class);
     private static final AutoLock LOCK = new AutoLock();
 
+    /**
+     * The init parameter name used to define {@link ServletContext} attribute used to share the {@link WebSocketMapping}.
+     */
+    public static final String MAPPING_ATTRIBUTE_INIT_PARAM = "jetty.websocket.WebSocketMapping";
+
+    /**
+     * Return any {@link WebSocketUpgradeFilter} already present on the {@link ServletContext}.
+     *
+     * @param servletContext the {@link ServletContext} to use.
+     * @return the configured default {@link WebSocketUpgradeFilter} instance.
+     */
     private static FilterHolder getFilter(ServletContext servletContext)
     {
         ContextHandler contextHandler = Objects.requireNonNull(ContextHandler.getContextHandler(servletContext));
         ServletHandler servletHandler = contextHandler.getChildHandlerByClass(ServletHandler.class);
-
         for (FilterHolder holder : servletHandler.getFilters())
         {
             if (holder.getInitParameter(MAPPING_ATTRIBUTE_INIT_PARAM) != null)
                 return holder;
         }
-
         return null;
     }
 
     /**
-     * Configure the default WebSocketUpgradeFilter.
-     *
-     * <p>
-     * This will return the default {@link WebSocketUpgradeFilter} on the
-     * provided {@link ServletContext}, creating the filter if necessary.
+     * Ensure a {@link WebSocketUpgradeFilter} is available on the provided {@link ServletContext},
+     * a new filter will added if one does not already exist.
      * </p>
      * <p>
      * The default {@link WebSocketUpgradeFilter} is also available via
      * the {@link ServletContext} attribute named {@code org.eclipse.jetty.websocket.server.WebSocketUpgradeFilter}
      * </p>
      *
-     * @param servletContext the {@link ServletContext} to use
-     * @return the configured default {@link WebSocketUpgradeFilter} instance
+     * @param servletContext the {@link ServletContext} to use.
+     * @return the configured default {@link WebSocketUpgradeFilter} instance.
      */
     public static FilterHolder ensureFilter(ServletContext servletContext)
     {
@@ -132,8 +137,6 @@ public static FilterHolder ensureFilter(ServletContext servletContext)
         }
     }
 
-    public static final String MAPPING_ATTRIBUTE_INIT_PARAM = "org.eclipse.jetty.websocket.util.server.internal.WebSocketMapping.key";
-
     private final Configuration.ConfigurationCustomizer defaultCustomizer = new Configuration.ConfigurationCustomizer();
     private WebSocketMapping mapping;
 
@@ -174,10 +177,9 @@ public void init(FilterConfig config) throws ServletException
         final ServletContext context = config.getServletContext();
 
         String mappingKey = config.getInitParameter(MAPPING_ATTRIBUTE_INIT_PARAM);
-        if (mappingKey != null)
-            mapping = WebSocketMapping.ensureMapping(context, mappingKey);
-        else
-            mapping = new WebSocketMapping(WebSocketServerComponents.getWebSocketComponents(context));
+        if (mappingKey == null)
+            throw new ServletException("the WebSocketMapping init param must be set");
+        mapping = WebSocketMapping.ensureMapping(context, mappingKey);
 
         String max = config.getInitParameter("idleTimeout");
         if (max == null)

diff --git a/...rver/src/main/java/org/eclipse/jetty/websocket/util/server/internal/WebSocketMapping.java b/...rver/src/main/java/org/eclipse/jetty/websocket/util/server/internal/WebSocketMapping.java
@@ -63,7 +63,6 @@ public class WebSocketMapping implements Dumpable, LifeCycle.Listener
     public static WebSocketMapping getMapping(ServletContext servletContext, String mappingKey)
     {
         Object mappingObject = servletContext.getAttribute(mappingKey);
-
         if (mappingObject != null)
         {
             if (mappingObject instanceof WebSocketMapping)
@@ -86,7 +85,6 @@ public WebSocketCreator getMapping(PathSpec pathSpec)
     public static WebSocketMapping ensureMapping(ServletContext servletContext, String mappingKey)
     {
         WebSocketMapping mapping = getMapping(servletContext, mappingKey);
-
         if (mapping == null)
         {
             mapping = new WebSocketMapping(WebSocketServerComponents.getWebSocketComponents(servletContext));
@@ -135,7 +133,7 @@ else if (rawSpec.startsWith("uri-template|"))
         throw new IllegalArgumentException("Unrecognized path spec syntax [" + rawSpec + "]");
     }
 
-    public static final String DEFAULT_KEY = "org.eclipse.jetty.websocket.util.server.internal.WebSocketMapping";
+    public static final String DEFAULT_KEY = "jetty.websocket.defaultMapping";
 
     private final PathMappings<Negotiator> mappings = new PathMappings<>();
     private final WebSocketComponents components;