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.

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

Fix Secure javax.websocket Client examples #4035

Merged

Conversation

lachlan-roberts
Copy link
Contributor

Signed-off-by: Lachlan Roberts lachlan@webtide.com

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Copy link
Contributor Author

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

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

@joakime this still has some problems.

I also noticed that running the test we do not get a close response and just close with close code 1006. But in the 9.4.x version of the tests we do get a 1000 response from the server. Not sure why this is happening.

@@ -92,7 +92,7 @@ public static void main(String[] args)
public static WebSocketContainer getConfiguredWebSocketContainer() throws Exception
{
URL jettyHttpClientConfigUrl = Thread.currentThread().getContextClassLoader()
.getResource("examples/jetty-websocket-httpclient.xml");
.getResource("jetty-websocket-httpclient.xml");
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 XmlBasedHttpClientProvider could not find the xml file when it was in examples/ but worked when moved up a directory to resources, so I think that the ClassLoader scope stuff below doesn't work.

If you replace this entire method with just ContainerProvider.getWebSocketContainer() it works as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for not putting that into the src/test/resources root is because that change would make all tests would then use this jetty-websocket-httpclient.xml
I was limiting the scope of it's usage with all of that classloader nonsense to only the 2 Examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out the reason the xml does not work in src/test/resources/examples is because in jetty-10 we lazily create the HttpClient so that the HttpClientProvider is run during the call to client.connectToServer(echoEndpoint, clientEndpointConfig, echoUri); in main and not in the ThreadClassLoaderScope try block below.

@lachlan-roberts lachlan-roberts marked this pull request as ready for review August 29, 2019 01:15
@lachlan-roberts
Copy link
Contributor Author

maybe these examples should be junit tests so we know they are tested and working

@joakime
Copy link
Contributor

joakime commented Aug 29, 2019

I'm not opposed to having tests.
But I'd like to keep the examples as is, as they help many individuals working with SSL on websocket.
They will be referenced on the documentation too (when I get around to it)

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@joakime joakime added this to the 10.0.x milestone Sep 3, 2019
@lachlan-roberts
Copy link
Contributor Author

@joakime can you review
I have removed the ClassLoader tricks as there are no as the HttpClient is lazily created in the client.connectToServer() call.

Alternatively I could just put this around the main test in the code.

try (ThreadClassLoaderScope ignore = new ThreadClassLoaderScope(classLoader))	
{	
    ...
}

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

LGTM

@joakime joakime changed the title fix secure javax websocket client examples Fix Secure javax.websocket Client examples Sep 17, 2019
@lachlan-roberts lachlan-roberts merged commit f10ff81 into jetty-10.0.x Sep 18, 2019
@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-secureJavaxWSClientExamples branch September 18, 2019 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants