Skip to content

Allow TCPSocket to bind to specific local address#3495

Closed
maxpowa wants to merge 2 commits intocrystal-lang:masterfrom
maxpowa:tcpsocket-bind-local
Closed

Allow TCPSocket to bind to specific local address#3495
maxpowa wants to merge 2 commits intocrystal-lang:masterfrom
maxpowa:tcpsocket-bind-local

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
Collaborator

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.

6 participants