-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Fix for #3229 #4152
Fix for #3229 #4152
Conversation
This is based on specs created by @yorickpeterse for the rubysl/rubysl-socket move to a pure-ruby socket lib. Among the changes thusfar: * Many improvements to Addrinfo, including leveraging the JDK APIs better and having less state. More address types work correctly now. * Socket now handles more types of sockets, including servers. * Improvements to addressing behavior across all socket types. * In-progress refactoring of socket classes to be reusable from Socket grab-bag. At the moment there are around 565 specs tagged.
Will be made in the separate PR
Also tested using specs from ruby/spec#287
Fix regression exposed by jruby/test_addrinfo test
Remove (un)pack_sockaddr_un specs from excludes
Bring back fallback for unbinded socket
@@ -46,51 +48,37 @@ | |||
* @author <a href="mailto:ola.bini@ki.se">Ola Bini</a> | |||
*/ | |||
@JRubyClass(name="IPSocket", parent="BasicSocket") | |||
public class RubyIPSocket extends RubyBasicSocket { | |||
public abstract class RubyIPSocket extends RubyBasicSocket { |
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.
IPSocket
is still allocate
-able ... not that it matters much (probably doesn't at all)
2.3.1 :029 > IPSocket.new
NoMethodError: undefined method `initialize' for #<IPSocket:0x0000000128ed70>
from (irb):29:in `new'
from (irb):29
from /opt/local/rvm/rubies/ruby-2.3.1/bin/irb:11:in `<main>'
2.3.1 :030 > IPSocket.allocate
=> #<IPSocket:0x000000012574b0>
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.
I presume this means you could extend IPSocket and make your own...not that I know how you'd do that with so much hidden in C.
I agree we should probably undo this change.
This is a big PR! Well done stitching your commits and my commits together! I must raise the issue of whitespace again, though. You have a number of places here changing nothing but whitespace. In such a big PR I don't expect you to go back and fix that, but please be mindful of it in the future and try to set your IDE to not make spurious formatting changes. I'll review. |
EPIC...I did not spend much time on this but landing sooner and getting it pushed through the meat grinder (e.g. we need some bake + expanded testing). One other tiny note: context.getRuntime() is fine but it is ok to do context.runtime if you want. We do this in so many places now I think it is an acceptable pattern to access the field. |
@kares, @headius done. Also reverted for JRuby after fix: >> BasicSocket.allocate
=> #<BasicSocket:0x1f7030a6>
>> BasicSocket.new
NoMethodError: undefined method `initialize' for #<BasicSocket:0x396f6598>
>> IPSocket.allocate
=> #<IPSocket:0x394e1a0f>
>> IPsocket.new
NoMethodError: undefined method `initialize' for #<IPSocket:0x1d29cf23> MRI 2.3.1: [1] pry(main)> BasicSocket.allocate
=> #<BasicSocket:0x007fb4eb5292a0>
[2] pry(main)> BasicSocket.new
NoMethodError: undefined method `initialize' for #<BasicSocket:0x007fb4eb4f0950>
from (pry):2:in `new'
[3] pry(main)> IPSocket.allocate
=> #<IPSocket:0x007fb4eb4bb408>
[4] pry(main)> IPSocket.new
NoMethodError: undefined method `initialize' for #<IPSocket:0x007fb4e9c5f918> Related to space trimming and |
4706ad6
to
8802bb0
Compare
Make RubyBasicSocket and RubyIPSocket allocatable to match MRI behaviour
8802bb0
to
7275cef
Compare
@eregon I think we need to address the rubysl-socket spec question. The specs there are considerably more complete, and appear to be licensed BSD. I think we should pull them into ruby/spec, so we can all start running them as part of that suite. |
@headius I missed this message, but essentially I think the simplest for now is to integrate the rubysl-socket specs in CI separately, as there is an overlap between both kind of specs and I'm unsure what to do LICENSE-wise. |
Almost full
Addrinfo
support and many other sockets-related improvements.fixes #3229