-
Notifications
You must be signed in to change notification settings - Fork 188
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
Coercion fixes for TCPServer.new #1780
Conversation
Hello Alan Wu, thanks for contributing a PR to our project! We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address xrxr -(at)- users -(dot)- noreply -(dot)- github -(dot)- com. You can sign it at that link. If you think you've already signed it, please comment below and we'll check. |
Alan Wu has signed the Oracle Contributor Agreement (based on email address xrxr -(at)- users -(dot)- noreply -(dot)- github -(dot)- com) so can contribute to this repository. |
18a09d3
to
bec246b
Compare
lib/truffle/socket/tcp_server.rb
Outdated
rescue ArgumentError | ||
raise SocketError, "invalid port number: #{host}" | ||
end | ||
end | ||
|
||
host = nil | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we could model this more simply by using:
def initialize(host = nil, service)
It seems that if we are using just 1 argument, it's always for the service and host being nil
/''
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I didn't think of using a trailing required arg. Actually, it seems that using
def initialize(host = nil, service)
we can simply delegate all the arguments to Socket
like:
diff --git a/lib/truffle/socket/tcp_server.rb b/lib/truffle/socket/tcp_server.rb
index dcbb7dbeff..76e6e86197 100644
--- a/lib/truffle/socket/tcp_server.rb
+++ b/lib/truffle/socket/tcp_server.rb
@@ -27,37 +27,9 @@
class TCPServer < TCPSocket
NOT_PASSED = Object.new
- def initialize(host, service = NOT_PASSED)
+ def initialize(host = nil, service)
@no_reverse_lookup = self.class.do_not_reverse_lookup
- if NOT_PASSED.equal?(service)
- service =
- if host.is_a?(Integer)
- host
- else
- string_service = Truffle::Socket.coerce_to_string(host)
- begin
- Integer(string_service)
- rescue ArgumentError
- raise SocketError, "invalid port number: #{host}"
- end
- end
-
- host = nil
- end
-
- service = 0 if service.nil?
-
- unless service.is_a?(Integer)
- service = Truffle::Socket.coerce_to_string(service)
- end
-
- if host
- host = Truffle::Socket.coerce_to_string(host)
- else
- host = ''
- end
-
remote_addrs = Socket
.getaddrinfo(host, service, :UNSPEC, :STREAM, 0, Socket::AI_PASSIVE)
Does this sound better? MRI does something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sounds great, could you update the PR with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
@server = TCPServer.new(port) | ||
addr = @server.addr | ||
addr[1].should be_kind_of(Integer) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specs look great 👍
Handle nil and empty string and coerce the port number the way MRI does.
bec246b
to
21518e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, a lot less code and a lot more correct :)
PullRequest: truffleruby/1107
Benoit suggested to me to use post args at oracle#1780 which lead me to take another look at Shopify/truffleruby-shopify#41 The original code that was there seem to be overly complicated with respect to coercion when `getaddrinfo` already does what we need. This is also how MRI does it.
Handle nil and empty string and coerce the port number the way MRI does.
Shopify#1