Issue #3730 - Cleaning up Scopes in WebSocketClient

jetty-websocket/javax-websocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/ClientContainer.java

@@ -57,7 +57,6 @@
 import org.eclipse.jetty.websocket.common.WebSocketSession;
 import org.eclipse.jetty.websocket.common.WebSocketSessionListener;
 import org.eclipse.jetty.websocket.common.scopes.DelegatedContainerScope;
-import org.eclipse.jetty.websocket.common.scopes.SimpleContainerScope;
 import org.eclipse.jetty.websocket.common.scopes.WebSocketContainerScope;
 import org.eclipse.jetty.websocket.jsr356.annotations.AnnotatedEndpointScanner;
 import org.eclipse.jetty.websocket.jsr356.client.AnnotatedClientEndpointMetadata;
@@ -109,9 +108,7 @@ public class ClientContainer extends ContainerLifeCycle implements WebSocketCont
     public ClientContainer()
     {
         // This constructor is used with Standalone JSR Client usage.
-        this(new SimpleContainerScope(WebSocketPolicy.newClientPolicy()));
-        client.setDaemon(true);
-        client.addManaged(client.getHttpClient());
+        this(new WebSocketClient());
     }
 
     /**
@@ -123,7 +120,7 @@ public ClientContainer()
      */
     public ClientContainer(final HttpClient httpClient)
     {
-        this(new SimpleContainerScope(WebSocketPolicy.newClientPolicy()), httpClient);
+        this(new WebSocketClient(httpClient));
     }
 
     /**
@@ -134,7 +131,6 @@ public ClientContainer(final HttpClient httpClient)
     public ClientContainer(final WebSocketContainerScope scope)
     {
         this(scope, null);
-        client.addManaged(client.getHttpClient());
     }
 
     /**
@@ -187,8 +183,12 @@ protected ClientContainer(final WebSocketContainerScope scope, final HttpClient
      */
     public ClientContainer(WebSocketClient client)
     {
+        Objects.requireNonNull(client, "WebSocketClient");
         this.scopeDelegate = client;
         this.client = client;
+        addBean(this.client);
+        this.client.setEventDriverFactory(new JsrEventDriverFactory(scopeDelegate));
+        this.client.setSessionFactory(new JsrSessionFactory(this));
         this.internalClient = false;
 
         this.endpointClientMetadataCache = new ConcurrentHashMap<>();

jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java

@@ -58,7 +58,6 @@
 import org.eclipse.jetty.websocket.common.WebSocketSessionListener;
 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;
 
 /**
@@ -77,26 +76,26 @@ public class WebSocketClient extends ContainerLifeCycle implements WebSocketCont
     // WebSocket Specifics
     private final WebSocketPolicy policy;
     private final WebSocketExtensionFactory extensionRegistry;
-    private final EventDriverFactory eventDriverFactory;
-    private final SessionFactory sessionFactory;
     private final SessionTracker sessionTracker = new SessionTracker();
     private final List<WebSocketSessionListener> sessionListeners = new ArrayList<>();
+    private EventDriverFactory eventDriverFactory;
+    private SessionFactory sessionFactory;
 
     // defaults to true for backwards compatibility
     private boolean stopAtShutdown = true;
 
     /**
-     * Instantiate a WebSocketClient with defaults
+     * Instantiate a WebSocketClient with defaults.
      */
     public WebSocketClient()
     {
-        this((HttpClient)null);
+        this(HttpClientProvider.get(null), null);
     }
 
     /**
-     * Instantiate a WebSocketClient using HttpClient for defaults
+     * Instantiate a WebSocketClient using provided HttpClient.
      *
-     * @param httpClient the HttpClient to base internal defaults off of
+     * @param httpClient the HttpClient to use for WebSocketClient.
      */
     public WebSocketClient(HttpClient httpClient)
     {
@@ -106,64 +105,90 @@ public WebSocketClient(HttpClient httpClient)
     /**
      * Instantiate a WebSocketClient using HttpClient for defaults
      *
-     * @param httpClient the HttpClient to base internal defaults off of
-     * @param objectFactory the DecoratedObjectFactory for all client instantiated classes
+     * @param httpClient the HttpClient that underlying WebSocket client uses
+     * @param decoratedObjectFactory the DecoratedObjectFactory for all client instantiated classes
      */
-    public WebSocketClient(HttpClient httpClient, DecoratedObjectFactory objectFactory)
+    public WebSocketClient(HttpClient httpClient, DecoratedObjectFactory decoratedObjectFactory)
     {
-        this(new SimpleContainerScope(new WebSocketPolicy(WebSocketBehavior.CLIENT), null, null, null, objectFactory), null, null, httpClient);
+        this.httpClient = Objects.requireNonNull(httpClient, "HttpClient");
+
+        addBean(httpClient);
+        addBean(sessionTracker);
+        addSessionListener(sessionTracker);
+
+        // Always a pristine Client policy
+        this.policy = WebSocketPolicy.newClientPolicy();
+        // We do not support late binding of DecoratedObjectFactory in this WebSocketClient
+        DecoratedObjectFactory objectFactory = decoratedObjectFactory == null ? new DecoratedObjectFactory() : decoratedObjectFactory;
+        this.objectFactorySupplier = () -> objectFactory;
+
+        this.extensionRegistry = new WebSocketExtensionFactory(this);
+        addBean(extensionRegistry);
+
+        this.eventDriverFactory = new EventDriverFactory(this);
+        this.sessionFactory = new WebSocketSessionFactory(this);
     }
 
     /**
      * Create a new WebSocketClient
      *
      * @param sslContextFactory ssl context factory to use on the internal {@link HttpClient}
+     * @deprecated use {@link #WebSocketClient(HttpClient)} instead
      */
+    @Deprecated
     public WebSocketClient(SslContextFactory sslContextFactory)
     {
-        this(sslContextFactory, null, null);
+        this(newHttpClient(sslContextFactory, null, null), null);
     }
 
     /**
      * Create a new WebSocketClient
      *
      * @param executor the executor to use on the internal {@link HttpClient}
+     * @deprecated use {@link #WebSocketClient(HttpClient)} instead
      */
+    @Deprecated
     public WebSocketClient(Executor executor)
     {
-        this(null, executor, null);
+        this(newHttpClient(null, executor, null), null);
     }
 
     /**
      * Create a new WebSocketClient
      *
      * @param bufferPool byte buffer pool to use on the internal {@link HttpClient}
+     * @deprecated use {@link #WebSocketClient(HttpClient)} instead
      */
+    @Deprecated
     public WebSocketClient(ByteBufferPool bufferPool)
     {
-        this(null, null, bufferPool);
+        this(newHttpClient(null, null, bufferPool), null);
     }
 
     /**
      * Create a new WebSocketClient
      *
      * @param sslContextFactory ssl context factory to use on the internal {@link HttpClient}
      * @param executor the executor to use on the internal {@link HttpClient}
+     * @deprecated use {@link #WebSocketClient(HttpClient)} instead
      */
+    @Deprecated
     public WebSocketClient(SslContextFactory sslContextFactory, Executor executor)
     {
-        this(sslContextFactory, executor, null);
+        this(newHttpClient(sslContextFactory, executor, null), null);
     }
 
     /**
      * Create WebSocketClient other Container Scope, to allow sharing of
      * internal features like Executor, ByteBufferPool, SSLContextFactory, etc.
      *
      * @param scope the Container Scope
+     * @deprecated use {@link #WebSocketClient(HttpClient)} instead
      */
+    @Deprecated
     public WebSocketClient(WebSocketContainerScope scope)
     {
-        this(scope, null, null, null);
+        this(newHttpClient(scope.getSslContextFactory(), scope.getExecutor(), scope.getBufferPool()), scope.getObjectFactory());
     }
 
     /**
@@ -173,10 +198,22 @@ public WebSocketClient(WebSocketContainerScope scope)
      * @param scope the Container Scope
      * @param sslContextFactory SSL ContextFactory to use in preference to one from
      * {@link WebSocketContainerScope#getSslContextFactory()}
+     * @deprecated use {@link #WebSocketClient(HttpClient)} instead
      */
+    @Deprecated
     public WebSocketClient(WebSocketContainerScope scope, SslContextFactory sslContextFactory)
     {
-        this(sslContextFactory, scope.getExecutor(), scope.getBufferPool(), scope.getObjectFactory());
+        /* This is constructor is in an awful place, it's got a signature that has a scope,
+         * a concept that javax.websocket ServerContainer uses to share its buffer pools / executors / etc
+         * with the underlying HttpClient.
+         * This means that the constructor should go through the HttpClientProvider.get(scope) behaviors.
+         * but it also has an arbitrary SslContextFactory parameter, which isn't in the scope that
+         * HttpClientProvider uses.
+         * Since this isn't used by Jetty's implementation of the javax.websocket ServerContainer
+         * this behavior has been changed to be non-scoped so as to be able to use the provided
+         * SslContextFactory for the underlying HttpClient instance.
+         */
+        this(newHttpClient(sslContextFactory, scope.getExecutor(), scope.getBufferPool()), scope.getObjectFactory());
     }
 
     /**
@@ -186,25 +223,12 @@ public WebSocketClient(WebSocketContainerScope scope, SslContextFactory sslConte
      * @param sslContextFactory shared SSL ContextFactory
      * @param executor shared Executor
      * @param bufferPool shared ByteBufferPool
+     * @deprecated use {@link #WebSocketClient(HttpClient)} instead
      */
+    @Deprecated
     public WebSocketClient(SslContextFactory sslContextFactory, Executor executor, ByteBufferPool bufferPool)
     {
-        this(sslContextFactory, executor, bufferPool, null);
-    }
-
-    /**
-     * Create WebSocketClient using sharing instances of SSLContextFactory
-     * Executor, and ByteBufferPool
-     *
-     * @param sslContextFactory shared SSL ContextFactory
-     * @param executor shared Executor
-     * @param bufferPool shared ByteBufferPool
-     * @param objectFactory shared DecoratedObjectFactory
-     */
-    private WebSocketClient(SslContextFactory sslContextFactory, Executor executor, ByteBufferPool bufferPool, DecoratedObjectFactory objectFactory)
-    {
-        this(new SimpleContainerScope(new WebSocketPolicy(WebSocketBehavior.CLIENT), bufferPool, executor, sslContextFactory, objectFactory));
-        addBean(this.httpClient);
+        this(newHttpClient(sslContextFactory, executor, bufferPool), null);
     }
 
     /**
@@ -214,10 +238,15 @@ private WebSocketClient(SslContextFactory sslContextFactory, Executor executor,
      * @param scope the Container Scope
      * @param eventDriverFactory the EventDriver Factory to use
      * @param sessionFactory the SessionFactory to use
+     * @deprecated use {@link WebSocketClient#WebSocketClient(WebSocketContainerScope, EventDriverFactory, SessionFactory, HttpClient)} instead
      */
+    @Deprecated
     public WebSocketClient(final WebSocketContainerScope scope, EventDriverFactory eventDriverFactory, SessionFactory sessionFactory)
     {
-        this(scope, eventDriverFactory, sessionFactory, null);
+        /* Nothing in Jetty uses this constructor anymore.
+         * It's left in for backwards compat reasons.
+         */
+        this(scope, eventDriverFactory, sessionFactory, HttpClientProvider.get(scope));
     }
 
     /**
@@ -231,30 +260,33 @@ public WebSocketClient(final WebSocketContainerScope scope, EventDriverFactory e
      */
     public WebSocketClient(final WebSocketContainerScope scope, EventDriverFactory eventDriverFactory, SessionFactory sessionFactory, HttpClient httpClient)
     {
-        if (httpClient == null)
-        {
-            this.httpClient = HttpClientProvider.get(scope);
-            addBean(this.httpClient);
-        }
-        else
-        {
-            this.httpClient = httpClient;
-        }
+        this.httpClient = httpClient == null ? HttpClientProvider.get(scope) : httpClient;
+        addBean(this.httpClient);
 
         addBean(sessionTracker);
         addSessionListener(sessionTracker);
 
         // Ensure we get a Client version of the policy.
         this.policy = scope.getPolicy().delegateAs(WebSocketBehavior.CLIENT);
-        // Support Late Binding of Object Factory (for CDI)
-        this.objectFactorySupplier = () -> scope.getObjectFactory();
+
+        // Support Late Binding of DecoratedObjectFactory (that CDI establishes in its own servlet context listeners)
+        this.objectFactorySupplier = scope::getObjectFactory;
+
         this.extensionRegistry = new WebSocketExtensionFactory(this);
         addBean(extensionRegistry);
 
         this.eventDriverFactory = eventDriverFactory == null ? new EventDriverFactory(this) : eventDriverFactory;
         this.sessionFactory = sessionFactory == null ? new WebSocketSessionFactory(this) : sessionFactory;
     }
 
+    private static HttpClient newHttpClient(SslContextFactory sslContextFactory, Executor executor, ByteBufferPool bufferPool)
+    {
+        HttpClient httpClient = new HttpClient(sslContextFactory);
+        httpClient.setExecutor(executor);
+        httpClient.setByteBufferPool(bufferPool);
+        return httpClient;
+    }
+
     public Future<Session> connect(Object websocket, URI toUri) throws IOException
     {
         ClientUpgradeRequest request = new ClientUpgradeRequest(toUri);
@@ -347,6 +379,16 @@ public Future<Session> connect(Object websocket, URI toUri, ClientUpgradeRequest
         return wsReq.sendAsync();
     }
 
+    public void setEventDriverFactory(EventDriverFactory eventDriverFactory)
+    {
+        this.eventDriverFactory = eventDriverFactory;
+    }
+
+    public void setSessionFactory(SessionFactory sessionFactory)
+    {
+        this.sessionFactory = sessionFactory;
+    }
+
     @Override
     protected void doStart() throws Exception
     {

jetty-websocket/websocket-client/src/test/java/org/eclipse/jetty/websocket/client/WebSocketClientInitTest.java

@@ -55,7 +55,6 @@ public void testInit_HttpClient_StartedOutside() throws Exception
                 assertThat("HttpClient started", http.isStarted(), is(true));
 
                 HttpClient httpBean = ws.getBean(HttpClient.class);
-                assertThat("HttpClient should not be found in WebSocketClient", httpBean, nullValue());
                 assertThat("HttpClient bean is managed", ws.isManaged(httpBean), is(false));
                 assertThat("WebSocketClient should not be found in HttpClient", http.getBean(WebSocketClient.class), nullValue());
             }