Issue #5828 - allow HttpClient to be used with JavaxWebSocketClientContainerProvider #5952
Conversation
…ntainerProvider Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…5828-JavaxWebSocketHttpClient
| { | ||
| try | ||
| { | ||
| clientContainer.start(); | ||
| container.start(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| throw new RuntimeException("Unable to start Client Container", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you replace this block with LifeCycle.start(container)?
| @@ -50,31 +51,43 @@ public static void stop(WebSocketContainer container) throws Exception | |||
| * </p> | |||
| */ | |||
| @Override | |||
| protected WebSocketContainer getContainer() | |||
| public WebSocketContainer getContainer() | |||
There was a problem hiding this comment.
Why did you open it from protected to public?
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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/eclipse-ee4j/websocket-api/issues/212 | ||
| private WebSocketContainer registerShutdown(JavaxWebSocketClientContainer container) | ||
| { | ||
| // Register as JVM runtime shutdown hook? |
There was a problem hiding this comment.
Can you remove the question mark at the end of this comment?
| JavaxWebSocketClientContainer clientContainer = new JavaxWebSocketClientContainer(); | ||
| registerShutdown(clientContainer); | ||
| return clientContainer; | ||
| } |
There was a problem hiding this comment.
Don't duplicate this code... can you call getContainer(null) instead?
There was a problem hiding this comment.
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.
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
…lientContainer Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
| @@ -50,31 +51,43 @@ public static void stop(WebSocketContainer container) throws Exception | |||
| * </p> | |||
| */ | |||
| @Override | |||
| protected WebSocketContainer getContainer() | |||
| public WebSocketContainer getContainer() | |||
There was a problem hiding this comment.
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.
…ected. Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts
deleted the
jetty-10.0.x-5828-JavaxWebSocketHttpClient
branch
closes #5828