Skip to content

Commit

Permalink
Merge pull request #4423 from eclipse/jetty-9.4.x-3730-websocket-scop…
Browse files Browse the repository at this point in the history 
…e-cleanup

Issue #3730 - Cleaning up Scopes in WebSocketClient
  • Loading branch information
@joakime
joakime authored Dec 19, 2019
2 parents af7cb94 + c2c2c56 commit cef67d9
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 52 deletions.
12 changes: 6 additions & 6 deletions ...bsocket-client-impl/src/main/java/org/eclipse/jetty/websocket/jsr356/ClientContainer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}

/**
Expand All @@ -123,7 +120,7 @@ public ClientContainer()
*/
public ClientContainer(final HttpClient httpClient)
{
this(new SimpleContainerScope(WebSocketPolicy.newClientPolicy()), httpClient);
this(new WebSocketClient(httpClient));
}

/**
Expand All @@ -134,7 +131,6 @@ public ClientContainer(final HttpClient httpClient)
public ClientContainer(final WebSocketContainerScope scope)
{
this(scope, null);
client.addManaged(client.getHttpClient());
}

/**
Expand Down Expand Up @@ -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<>();
Expand Down
132 changes: 87 additions & 45 deletions ...et/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/WebSocketClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -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)
{
Expand All @@ -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());
}

/**
Expand All @@ -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());
}

/**
Expand All @@ -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);
}

/**
Expand All @@ -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));
}

/**
Expand All @@ -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);
Expand Down Expand Up @@ -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
{
Expand Down
1 change: 0 additions & 1 deletion ...cket-client/src/test/java/org/eclipse/jetty/websocket/client/WebSocketClientInitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down

0 comments on commit cef67d9

Please sign in to comment.