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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented a custom DNS resolver, see #859 #906

Merged
merged 4 commits into from Jun 23, 2019
Merged

Implemented a custom DNS resolver, see #859 #906

merged 4 commits into from Jun 23, 2019

Conversation

maiph
Copy link
Contributor

@maiph maiph commented Jun 13, 2019

Fixes #859

Description

Implements a custom DNS resolver for WebSocketClient to use.

Related Issue

See #859

Motivation and Context

See #859

How Has This Been Tested?

Runned the already existing tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project. (I hope this is the new code style 馃槃)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed. (Issue256Test is failing)

Copy link
Collaborator

@PhilipRoman PhilipRoman left a comment

Choose a reason for hiding this comment

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

While I haven't yet reviewed the rest of this PR, there is at least one bug: it fails when URI does not contain a port. Code to reproduce:

import org.java_websocket.*;
import org.java_websocket.handshake.*;
import org.java_websocket.server.*;
import org.java_websocket.client.*;
import java.net.*;
import javax.net.*;

public class Main {
	public static void main(String[] args) throws Exception {
		var server = new WebSocketServer(new InetSocketAddress(80)) {
			public void onError(WebSocket ws, Exception e) {e.printStackTrace();}
			public void onMessage(WebSocket ws, String msg) {System.out.println("Received " + msg);}
			public void onStart() {System.out.println("Server started!");}
			public void onOpen(WebSocket ws, ClientHandshake hs) {}
			public void onClose(WebSocket ws, int code, String msg, boolean remote) {}
		};
		server.start();
		var client = new WebSocketClient(new URI("ws://localhost")) {
			public void onMessage(String msg) {}
			public void onError(Exception e) {e.printStackTrace();}
			public void onClose(int code, String msg, boolean remote) {}
			public void onOpen(ServerHandshake hs) {System.out.println("Client started");}
		};
		client.connectBlocking();
		client.send("Message");
		client.close();
		server.stop();
	}
}

Expected output (using TooTallNate:master):

SLF4J: No SLF4J providers were found.
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#noProviders for further details.
Server started!
Client started
Server received: Message

Actual output (using maiph:dns-resolver):

SLF4J: No SLF4J providers were found.
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#noProviders for further details.
java.lang.IllegalArgumentException: port out of range:-1
        at java.base/java.net.InetSocketAddress.checkPort(InetSocketAddress.java:143)
        at java.base/java.net.InetSocketAddress.<init>(InetSocketAddress.java:188)
        at org.java_websocket.client.WebSocketClient.run(WebSocketClient.java:463)
        at java.base/java.lang.Thread.run(Thread.java:834)
Exception in thread "main" Server started!
org.java_websocket.exceptions.WebsocketNotConnectedException
        at org.java_websocket.WebSocketImpl.send(WebSocketImpl.java:630)
        at org.java_websocket.WebSocketImpl.send(WebSocketImpl.java:607)
        at org.java_websocket.client.WebSocketClient.send(WebSocketClient.java:412)
        at Main.main(Main.java:26)

URI::getPort() returns -1 when no port is provided, but InetSocketAddress(...) requires a port number between 0 and 65535. WebSocketClient::getPort() contains additional logic to fix this case.

Actually, it might be worth adding a similar program as a test case, because this issue will occur whenever someone uses uri.getPort() instead of this.getPort() (which isn't obvious for new contributors)

@maiph
Copy link
Contributor Author

maiph commented Jun 13, 2019

Should i assume the default ports based on the protocol then (ws=80 and wss=443) ?

@PhilipRoman
Copy link
Collaborator

I think this can be fixed by using this.getPort() instead of uri.getPort()

Copy link
Collaborator

@PhilipRoman PhilipRoman left a comment

Choose a reason for hiding this comment

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

DnsResolver uses 2 spaces instead of tab. Also, since it is only used on the client side, maybe move it to package org.java_websocket.client?

@marci4 marci4 added this to the Release 1.4.1 milestone Jun 14, 2019
Copy link
Collaborator

@marci4 marci4 left a comment

Choose a reason for hiding this comment

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

Could you please add the license text from LICENSE at the top of file?

@marci4 marci4 merged commit c9e7533 into TooTallNate:master Jun 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hot wo specify a custom DNS Resolver
3 participants