Issue #2655 - Applying changes requested from review.

+ WebSocketClient all constructors now delegate down into one implementation + NativeWebSocketServletConfiguration is now managed properly + WebSocketServletFactory can find Executor as bean too Signed-off-by: Joakim Erdfelt <[email protected]>
jetty · Jun 20, 2018 · 2e5f106 · 2e5f106
1 parent 870c87f
commit 2e5f106
Show file tree

Hide file tree

Showing 6 changed files with 95 additions and 66 deletions.
diff --git a/...c/test/java/org/eclipse/jetty/websocket/jsr356/server/DelayedStartClientOnServerTest.java b/...c/test/java/org/eclipse/jetty/websocket/jsr356/server/DelayedStartClientOnServerTest.java
@@ -247,7 +247,7 @@ public void testHttpClientThreads_AfterClientConnectTo() throws Exception
             assertThat("Response", response, startsWith("Connected to ws://"));
             List<String> threadNames = getThreadNames(server);
             assertNoHttpClientPoolThreads(threadNames);
-            assertThat("Threads", threadNames, hasItem(containsString("WebSocketContainer@")));
+            assertThat("Threads", threadNames, hasItem(containsString("WebSocketClient@")));
         }
         finally
         {

diff --git a/...et-client/src/main/java/org/eclipse/jetty/websocket/client/DefaultHttpClientProvider.java b/...et-client/src/main/java/org/eclipse/jetty/websocket/client/DefaultHttpClientProvider.java
@@ -21,6 +21,8 @@
 import java.util.concurrent.Executor;
 
 import org.eclipse.jetty.client.HttpClient;
+import org.eclipse.jetty.io.ByteBufferPool;
+import org.eclipse.jetty.io.MappedByteBufferPool;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.eclipse.jetty.util.thread.QueuedThreadPool;
 import org.eclipse.jetty.websocket.common.scopes.WebSocketContainerScope;
@@ -31,11 +33,13 @@ public static HttpClient newHttpClient(WebSocketContainerScope scope)
     {
         SslContextFactory sslContextFactory = null;
         Executor executor = null;
+        ByteBufferPool bufferPool = null;
 
         if (scope != null)
         {
             sslContextFactory = scope.getSslContextFactory();
             executor = scope.getExecutor();
+            bufferPool = scope.getBufferPool();
         }
 
         if (sslContextFactory == null)
@@ -53,6 +57,13 @@ public static HttpClient newHttpClient(WebSocketContainerScope scope)
             executor = threadPool;
         }
         client.setExecutor(executor);
+
+        if (bufferPool == null)
+        {
+            bufferPool = new MappedByteBufferPool();
+        }
+        client.setByteBufferPool(bufferPool);
+
         return client;
     }
 }
diff --git a/...et/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java b/...et/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java
@@ -29,14 +29,11 @@
 import java.util.Set;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Future;
-import java.util.concurrent.ThreadLocalRandom;
 import java.util.function.Supplier;
 
 import org.eclipse.jetty.client.HttpClient;
 import org.eclipse.jetty.io.ByteBufferPool;
-import org.eclipse.jetty.io.MappedByteBufferPool;
 import org.eclipse.jetty.util.DecoratedObjectFactory;
-import org.eclipse.jetty.util.DeprecationWarning;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.component.ContainerLifeCycle;
 import org.eclipse.jetty.util.log.Log;
@@ -58,6 +55,7 @@
 import org.eclipse.jetty.websocket.common.WebSocketSessionFactory;
 import org.eclipse.jetty.websocket.common.events.EventDriverFactory;
 import org.eclipse.jetty.websocket.common.extensions.WebSocketExtensionFactory;
+import org.eclipse.jetty.websocket.common.scopes.SimpleContainerScope;
 import org.eclipse.jetty.websocket.common.scopes.WebSocketContainerScope;
 
 /**
@@ -79,9 +77,6 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketCont
     private final EventDriverFactory eventDriverFactory;
     private final SessionFactory sessionFactory;
 
-    // ID Generator
-    private final int id = ThreadLocalRandom.current().nextInt();
-
     // defaults to true for backwards compatibility
     private boolean stopAtShutdown = true;
 
@@ -90,9 +85,7 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketCont
      */
     public WebSocketClient()
     {
-        // Create synthetic HttpClient
-        this(HttpClientProvider.get(null));
-        addBean(this.httpClient);
+        this((HttpClient)null);
     }
 
     /**
@@ -103,7 +96,7 @@ public WebSocketClient()
      */
     public WebSocketClient(HttpClient httpClient)
     {
-        this(httpClient,new DecoratedObjectFactory());
+        this(httpClient, null);
     }
 
     /**
@@ -116,48 +109,45 @@ public WebSocketClient(HttpClient httpClient)
      */
     public WebSocketClient(HttpClient httpClient, DecoratedObjectFactory objectFactory)
     {
-        this.httpClient = Objects.requireNonNull(httpClient, "HttpClient");
-        this.policy = new WebSocketPolicy(WebSocketBehavior.CLIENT);
-        final DecoratedObjectFactory decoratedObjectFactory = objectFactory != null ? objectFactory : newDecoratedObjectFactory();
-        this.objectFactorySupplier = () -> decoratedObjectFactory;
-        this.extensionRegistry = new WebSocketExtensionFactory(this);
-        this.eventDriverFactory = new EventDriverFactory(this);
-        this.sessionFactory = new WebSocketSessionFactory(this);
+        this(new SimpleContainerScope(new WebSocketPolicy(WebSocketBehavior.CLIENT), null, null, null, objectFactory), null, null, httpClient);
     }
 
     /**
      * Create a new WebSocketClient
      *
-     * @param executor
-     *            the executor to use
+     * @param sslContextFactory
+     *            ssl context factory to use
      * @deprecated use {@link #WebSocketClient(HttpClient)} instead
      */
     @Deprecated
-    public WebSocketClient(Executor executor)
+    public WebSocketClient(SslContextFactory sslContextFactory)
     {
-        this(null,executor);
+        this(sslContextFactory,null, null);
     }
 
     /**
      * Create a new WebSocketClient
      *
-     * @param bufferPool
-     *            byte buffer pool to use
+     * @param executor
+     *            the executor to use
+     * @deprecated use {@link #WebSocketClient(HttpClient)} instead
      */
-    public WebSocketClient(ByteBufferPool bufferPool)
+    public WebSocketClient(Executor executor)
     {
-        this(null,null,bufferPool);
+        this(null, executor, null);
     }
 
     /**
      * Create a new WebSocketClient
      *
-     * @param sslContextFactory
-     *            ssl context factory to use
+     * @param bufferPool
+     *            byte buffer pool to use
+     * @deprecated use {@link #WebSocketClient(HttpClient)} instead
      */
-    public WebSocketClient(SslContextFactory sslContextFactory)
+    @Deprecated
+    public WebSocketClient(ByteBufferPool bufferPool)
     {
-        this(sslContextFactory,null);
+        this(null, null, bufferPool);
     }
 
     /**
@@ -172,7 +162,7 @@ public WebSocketClient(SslContextFactory sslContextFactory)
     @Deprecated
     public WebSocketClient(SslContextFactory sslContextFactory, Executor executor)
     {
-        this(sslContextFactory,executor,new MappedByteBufferPool());
+        this(sslContextFactory, executor, null);
     }
 
     /**
@@ -184,7 +174,7 @@ public WebSocketClient(SslContextFactory sslContextFactory, Executor executor)
      */
     public WebSocketClient(WebSocketContainerScope scope)
     {
-        this(scope.getSslContextFactory(),scope.getExecutor(),scope.getBufferPool(),scope.getObjectFactory());
+        this(scope, null, null, null);
     }
 
     /**
@@ -199,7 +189,7 @@ public WebSocketClient(WebSocketContainerScope scope)
      */
     public WebSocketClient(WebSocketContainerScope scope, SslContextFactory sslContextFactory)
     {
-        this(sslContextFactory,scope.getExecutor(),scope.getBufferPool(),scope.getObjectFactory());
+        this(sslContextFactory, scope.getExecutor(), scope.getBufferPool(), scope.getObjectFactory());
     }
 
     /**
@@ -215,7 +205,7 @@ public WebSocketClient(WebSocketContainerScope scope, SslContextFactory sslConte
      */
     public WebSocketClient(SslContextFactory sslContextFactory, Executor executor, ByteBufferPool bufferPool)
     {
-        this(sslContextFactory,executor,bufferPool,new DecoratedObjectFactory());
+        this(sslContextFactory, executor, bufferPool, null);
     }
 
     /**
@@ -233,19 +223,8 @@ public WebSocketClient(SslContextFactory sslContextFactory, Executor executor, B
      */
     private WebSocketClient(SslContextFactory sslContextFactory, Executor executor, ByteBufferPool bufferPool, DecoratedObjectFactory objectFactory)
     {
-        this.httpClient = new HttpClient(sslContextFactory);
-        this.httpClient.setExecutor(executor);
-        this.httpClient.setByteBufferPool(bufferPool);
+        this(new SimpleContainerScope(new WebSocketPolicy(WebSocketBehavior.CLIENT), bufferPool, executor, sslContextFactory, objectFactory));
         addBean(this.httpClient);
-
-        this.policy = new WebSocketPolicy(WebSocketBehavior.CLIENT);
-        final DecoratedObjectFactory decoratedObjectFactory = objectFactory != null ? objectFactory : newDecoratedObjectFactory();
-        this.objectFactorySupplier = ()-> decoratedObjectFactory;
-
-        this.extensionRegistry = new WebSocketExtensionFactory(this);
-
-        this.eventDriverFactory = new EventDriverFactory(this);
-        this.sessionFactory = new WebSocketSessionFactory(this);
     }
 
     /**
@@ -279,7 +258,7 @@ public WebSocketClient(final WebSocketContainerScope scope, EventDriverFactory e
      */
     public WebSocketClient(final WebSocketContainerScope scope, EventDriverFactory eventDriverFactory, SessionFactory sessionFactory, HttpClient httpClient)
     {
-        if(httpClient == null)
+        if (httpClient == null)
         {
             this.httpClient = HttpClientProvider.get(scope);
             addBean(this.httpClient);
@@ -289,20 +268,14 @@ public WebSocketClient(final WebSocketContainerScope scope, EventDriverFactory e
             this.httpClient = httpClient;
         }
 
+        // Ensure we get a Client version of the policy.
         this.policy = scope.getPolicy().clonePolicy(WebSocketBehavior.CLIENT);
         // Support Late Binding of Object Factory (for CDI)
         this.objectFactorySupplier = () -> scope.getObjectFactory();
         this.extensionRegistry = new WebSocketExtensionFactory(this);
-
-        this.eventDriverFactory = eventDriverFactory;
-        this.sessionFactory = sessionFactory;
-    }
 
-    private DecoratedObjectFactory newDecoratedObjectFactory()
-    {
-        DecoratedObjectFactory objectFactory = new DecoratedObjectFactory();
-        objectFactory.addDecorator(new DeprecationWarning());
-        return objectFactory;
+        this.eventDriverFactory = eventDriverFactory == null ? new EventDriverFactory(this) : eventDriverFactory;
+        this.sessionFactory = sessionFactory == null ? new WebSocketSessionFactory(this) : sessionFactory;
     }
 
     public Future<Session> connect(Object websocket, URI toUri) throws IOException
@@ -758,11 +731,27 @@ public boolean isStopAtShutdown()
         return stopAtShutdown;
     }
 
+    @Override
+    public boolean equals(Object o)
+    {
+        if (this == o) return true;
+        if (!(o instanceof WebSocketClient)) return false;
+        WebSocketClient that = (WebSocketClient) o;
+        return Objects.equals(this.httpClient, that.httpClient) &&
+                Objects.equals(this.policy, that.policy);
+    }
+
+    @Override
+    public int hashCode()
+    {
+        return Objects.hash(httpClient, policy);
+    }
+
     @Override
     public String toString()
     {
         final StringBuilder sb = new StringBuilder("WebSocketClient@");
-        sb.append(Integer.toHexString(id));
+        sb.append(Integer.toHexString(hashCode()));
         sb.append("[httpClient=").append(httpClient);
         sb.append(",openSessions.size=");
         sb.append(getOpenSessions().size());

diff --git a/...-common/src/main/java/org/eclipse/jetty/websocket/common/scopes/SimpleContainerScope.java b/...-common/src/main/java/org/eclipse/jetty/websocket/common/scopes/SimpleContainerScope.java
@@ -23,9 +23,11 @@
 import org.eclipse.jetty.io.ByteBufferPool;
 import org.eclipse.jetty.io.MappedByteBufferPool;
 import org.eclipse.jetty.util.DecoratedObjectFactory;
+import org.eclipse.jetty.util.DeprecationWarning;
 import org.eclipse.jetty.util.component.ContainerLifeCycle;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
 import org.eclipse.jetty.util.thread.QueuedThreadPool;
+import org.eclipse.jetty.websocket.api.WebSocketBehavior;
 import org.eclipse.jetty.websocket.api.WebSocketPolicy;
 import org.eclipse.jetty.websocket.common.WebSocketSession;
 
@@ -54,23 +56,46 @@ public SimpleContainerScope(WebSocketPolicy policy, ByteBufferPool bufferPool, D
     }
 
     public SimpleContainerScope(WebSocketPolicy policy, ByteBufferPool bufferPool, Executor executor, DecoratedObjectFactory objectFactory)
+    {
+        this(policy, bufferPool, executor, null, objectFactory);
+    }
+
+    public SimpleContainerScope(WebSocketPolicy policy, ByteBufferPool bufferPool, Executor executor, SslContextFactory ssl, DecoratedObjectFactory objectFactory)
     {
         this.policy = policy;
         this.bufferPool = bufferPool;
 
         if (objectFactory == null)
         {
             this.objectFactory = new DecoratedObjectFactory();
+            this.objectFactory.addDecorator(new DeprecationWarning());
         }
         else
         {
             this.objectFactory = objectFactory;
         }
+
+        if(ssl == null)
+        {
+            this.sslContextFactory = new SslContextFactory();
+        }
+        else
+        {
+            this.sslContextFactory = ssl;
+        }
 
         if (executor == null)
         {
             QueuedThreadPool threadPool = new QueuedThreadPool();
-            String name = "WebSocketContainer@" + hashCode();
+            String behavior = "Container";
+            if (policy != null)
+            {
+                if (policy.getBehavior() == WebSocketBehavior.CLIENT)
+                    behavior = "Client";
+                else if (policy.getBehavior() == WebSocketBehavior.SERVER)
+                    behavior = "Server";
+            }
+            String name = String.format("WebSocket%s@%s", behavior, hashCode());
             threadPool.setName(name);
             threadPool.setDaemon(true);
             this.executor = threadPool;

diff --git a/...n/java/org/eclipse/jetty/websocket/server/NativeWebSocketServletContainerInitializer.java b/...n/java/org/eclipse/jetty/websocket/server/NativeWebSocketServletContainerInitializer.java
@@ -43,7 +43,7 @@ public static NativeWebSocketConfiguration getDefaultFrom(ServletContext context
             {
                 ContextHandler handler = ((ContextHandler.Context)context).getContextHandler();
                 // Let ContextHandler handle configuration lifecycle
-                handler.addBean(configuration);
+                handler.addManaged(configuration);
             }
         }
         return configuration;