Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #5828 - allow HttpClient to be used with JavaxWebSocketClientContainerProvider #5952

Merged
merged 5 commits into from
Feb 21, 2021
Merged

Issue #5828 - allow HttpClient to be used with JavaxWebSocketClientContainerProvider #5952

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7a72a6a
Issue #5828 - allow HttpClient to be used with JavaxWebSocketClientCo…
lachlan-roberts Dec 23, 2020
805d6ef
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
lachlan-roberts Feb 8, 2021
0e4ed5c
Issue #5828 - cleanups from review
lachlan-roberts Feb 15, 2021
b69f470
Issue #5828 - only use the HttpClient constructor for JavaxWebSocketC…
lachlan-roberts Feb 15, 2021
cee44ff
JavaxWebSocketClientContainerProvider getContainer should remain prot…
lachlan-roberts Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions ...ore-client/src/main/java/org/eclipse/jetty/websocket/core/client/WebSocketCoreClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ public WebSocketCoreClient(HttpClient httpClient, WebSocketComponents webSocketC
this.httpClient = httpClient;
this.components = webSocketComponents;
addBean(httpClient);
addBean(webSocketComponents);
}

public CompletableFuture<CoreSession> connect(FrameHandler frameHandler, URI wsUri) throws IOException
Expand Down
2 changes: 1 addition & 1 deletion jetty-websocket/websocket-javax-client/src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
exports org.eclipse.jetty.websocket.javax.client;
exports org.eclipse.jetty.websocket.javax.client.internal to org.eclipse.jetty.websocket.javax.server;

requires org.eclipse.jetty.client;
requires org.eclipse.jetty.websocket.core.client;
requires org.eclipse.jetty.websocket.javax.common;
requires transitive org.eclipse.jetty.client;
requires transitive jetty.websocket.api;

provides ContainerProvider with JavaxWebSocketClientContainerProvider;
Expand Down
35 changes: 24 additions & 11 deletions .../java/org/eclipse/jetty/websocket/javax/client/JavaxWebSocketClientContainerProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import javax.websocket.ContainerProvider;
import javax.websocket.WebSocketContainer;

import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.util.component.LifeCycle;
import org.eclipse.jetty.util.thread.ShutdownThread;
import org.eclipse.jetty.websocket.javax.client.internal.JavaxWebSocketClientContainer;
Expand Down Expand Up @@ -50,31 +51,43 @@ public static void stop(WebSocketContainer container) throws Exception
* </p>
*/
@Override
protected WebSocketContainer getContainer()
public WebSocketContainer getContainer()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you open it from protected to public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because with the new method we are setting the precedent that you can call these methods directly on the JavaxWebSocketClientContainer and not always go though the ServiceLoader using ContainerProvider.getWebSocketContainer().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This break people that override this method -- they will have protected in their code that will appear to restrict accessibility. Not many will override it, but still.

{
// See: https://github.com/javaee/websocket-spec/issues/212
// TODO: on multiple executions, do we warn?
// TODO: do we care?
// TODO: on multiple executions, do we share bufferPool/executors/etc?
// TODO: do we want to provide a non-standard way to configure to always return the same clientContainer based on a config somewhere? (system.property?)

JavaxWebSocketClientContainer clientContainer = new JavaxWebSocketClientContainer();
registerShutdown(clientContainer);
return clientContainer;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't duplicate this code... can you call getContainer(null) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementations are slightly different, new JavaxWebSocketClientContainer() will set the name of the HttpClient but new JavaxWebSocketClientContainer(null) won't do this.

But I don't think we should be doing this because it is done by the HttpClientProvider.


/**
* Get a new instance of a client {@link WebSocketContainer} which uses a supplied {@link HttpClient}.
* @param httpClient a pre-configured {@link HttpClient} to be used by the implementation.
* @see #getContainer()
*/
public WebSocketContainer getContainer(HttpClient httpClient)
{
JavaxWebSocketClientContainer clientContainer = new JavaxWebSocketClientContainer(httpClient);
registerShutdown(clientContainer);
return clientContainer;
}

// See: https://github.com/eclipse-ee4j/websocket-api/issues/212
private WebSocketContainer registerShutdown(JavaxWebSocketClientContainer container)
{
// Register as JVM runtime shutdown hook?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the question mark at the end of this comment?

ShutdownThread.register(clientContainer);
ShutdownThread.register(container);

if (!clientContainer.isStarted())
if (!container.isStarted())
{
try
{
clientContainer.start();
container.start();
}
catch (Exception e)
{
throw new RuntimeException("Unable to start Client Container", e);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace this block with LifeCycle.start(container)?


return clientContainer;
return container;
}
}
7 changes: 1 addition & 6 deletions ...java/org/eclipse/jetty/websocket/javax/client/internal/JavaxWebSocketClientContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,7 @@ public JavaxWebSocketClientContainer()
*/
public JavaxWebSocketClientContainer(final HttpClient httpClient)
{
this(new WebSocketComponents(), (wsComponents) ->
{
WebSocketCoreClient coreClient = new WebSocketCoreClient(httpClient, wsComponents);
coreClient.getHttpClient().setName("Javax-WebSocketClient@" + Integer.toHexString(coreClient.getHttpClient().hashCode()));
return coreClient;
});
this(new WebSocketComponents(), (wsComponents) -> new WebSocketCoreClient(httpClient, wsComponents));
}

public JavaxWebSocketClientContainer(WebSocketComponents components)
Expand Down