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 TCPSocket to bind to specific local address #3495

Closed
wants to merge 2 commits into from

Conversation

maxpowa
Copy link
Contributor

@maxpowa maxpowa commented Nov 2, 2016

Allows a TCPSocket to use a specific address for outgoing connections, for use in systems with more than a single interface. Adds two TCPSocket overloads to the existing instantiation methods, TCPSocket.open and TCPSocket.new, with additional parameters to allow specifying an IP and port combination to bind. Not sure how great the specs are, UDPSocket has much better testing for this but unfortunately TCPSocket cannot be instantiated in the same way as UDPSocket.

cc @RX14

sock.close
end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to add # ditto special comment so the other self.open you took the documentation from obtains the same one 😄

https://crystal-lang.org/docs/conventions/documenting_code.html

Copy link
Member

Choose a reason for hiding this comment

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

We could actually make this just def self.open(*args, **nargs) and then forward the arguments to new, so we only need one version of this method.

@maxpowa maxpowa force-pushed the tcpsocket-bind-local branch from fc5e86d to 963d8ea Compare November 3, 2016 16:26
@maxpowa maxpowa force-pushed the tcpsocket-bind-local branch 3 times, most recently from e2a8239 to 0edbc62 Compare November 3, 2016 17:17
@maxpowa maxpowa force-pushed the tcpsocket-bind-local branch from 0edbc62 to 53c85a3 Compare November 3, 2016 17:19
@ysbaddaden
Copy link
Contributor

I'm not sure this is so common that it's really worth adding more arguments to TCPSocket (and complicate it).

I've been considering to refactor Socket to look more like Ruby, where Socket has all the low level C-like methods (bind, listen, ...), so we have access to everything, and where TCPSocket and UDPSocket are limited, but simple, abstractions on top of it.

This way one could:

sock = Socket.new(Socket::Protocol::TCP)
sock.bind(local_host, local_port)
sock.connect(host, port)

Or by making the connection lazy:

sock = TCPSocket.new(host, port)
sock.bind(local_host, local_port)

@maxpowa
Copy link
Contributor Author

maxpowa commented Nov 3, 2016

I agree, Ruby/C style is much better but I wanted to avoid breaking anything with this patch. In order to have an explicit bind call an explicit connect call is also needed, which TCPSocket does not currently have (but UDPSocket does 😕).

@bararchy
Copy link
Contributor

bararchy commented Dec 7, 2016

@ysbaddaden , @maxpowa
In Ruby I like the different between the "client" and "server" helper wrappers around the socket class.

client: TCPSocket.new("127.0.0.1", 8080)
server: TCPServer.new(8080) (listens on 0.0.0.0 on port 8080) and TCPServer.new("127.0.0.1", 8080) (listens on 127.0.0.1 on port 8080)
raw socket: Socket.new
Shouldn't we conform to this standard ?

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.

None yet

6 participants