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

Allow using setServerNames on WebSocket #201

Closed
MinnDevelopment opened this issue Jul 19, 2020 · 3 comments · Fixed by #202
Closed

Allow using setServerNames on WebSocket #201

MinnDevelopment opened this issue Jul 19, 2020 · 3 comments · Fixed by #202

Comments

@MinnDevelopment
Copy link
Contributor

Currently, you have to use WebSocketFactory#setServerNames for SNI parameters. However, I want to use the factory for different connections to different domains. This requires doing synchronization on the factory only to set the server name:

synchronized (socketFactory) {
    String host = IOUtil.getHost(url);
    // null if the host is undefined, unlikely but we should handle it
    if (host != null)
        socketFactory.setServerName(host);
    else // practically should never happen
        socketFactory.setServerNames(null);
    socket = socketFactory.createSocket(url);
}

I have 3 solutions to this problem with decreasing complexity:

  1. Allow setting the server names on the socket rather than only the factory, this would require a few changes to the handling of the raw socket creation
  2. Allow passing the server names in createSocket to have an idempotent behavior
  3. Add a way to copy the factory, this should be fairly trivial to implement. For instance new WebSocketFactory(otherFactory) or similar. I would be willing to PR this.
@TakahikoKawasaki
Copy link
Owner

@MinnDevelopment Thank you. I'm interested in what your PR will look like 😀 . Please note that I'm a bit picky about beauty of source codes, so PRs that "just work" won't be merged 😅 . My programming style can be found in repositories in github.com/authlete and github.com/TakahikoKawasaki. Finally, I'm sorry I'm busy as a co-founder and programmer of Authlete, Inc., so it may take time for me to respond.

@MinnDevelopment
Copy link
Contributor Author

@TakahikoKawasaki I already have something but I haven't gotten around to testing it yet. I will try my best to adhere to the code style.

@TakahikoKawasaki
Copy link
Owner

@MinnDevelopment Thank you for your PR! nv-websocket-client 2.10 including your PR was released. I hope the release will appear in the Maven central repository soon.

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 a pull request may close this issue.

2 participants