Added Android-compatibile support for SNI in direct connections #109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
or-else
reviewed
Sep 19, 2017
| // Connect to the server (either a proxy or a WebSocket endpoint); overlay mSocket | ||
| // Ignore mConnectionTimeout | ||
| mSocket = mSocketFactory.createSocket(mAddress.getHostname(), mAddress.toInetSocketAddress().getPort()); | ||
| // mSocket.connect(mAddress.toInetSocketAddress(), mConnectionTimeout); |
There was a problem hiding this comment.
Why not add here mSocket.setSoTimeout(mConnectionTimeout);. It should solve the problem with timeout not being set.
There was a problem hiding this comment.
Or, even better,
mSocket = mSocketFactory.createSocket(); // This will create an unconnected socket
mSocket.setSoTimeout(mConnectionTimeout); // Set timeout
mSocket = mSocketFactory.createSocket(mSocket, mAddress.getHostname(), mAddress.toInetSocketAddress().getPort(), true); // Connect.
There was a problem hiding this comment.
Here is a relevant discussion https://stackoverflow.com/questions/7254389/create-sslsocket-by-sslsocketfactory-with-set-connection-timeout
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is not meant for direct merging, but rather for a discussion; the problem is that the API for SNI is available in Java8/Android API24 (the 'correct' implementation is in my branch https://github.com/ondrap/nv-websocket-client/tree/android-api24 ). This PR is rather slight refactoring that results in correct behaviour for direct connections on Android systems and possibly plain Java (tested with Java8). SNI in this case doesn't work for proxied connections as there is no API available to do that.
There is a slight drawback though, as there is no way to set connectionTimeout with the
createSocket()call, so the system default is used. I'm OK with that for my use, however I am not sure if such a change is appropriate for the library.