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.
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.
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 |
etehtsea
force-pushed
the
socket-next-re
branch
from
4706ad6
to
8802bb0
Compare
Make RubyBasicSocket and RubyIPSocket allocatable to match MRI behaviour
etehtsea
force-pushed
the
socket-next-re
branch
from
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
Addrinfosupport and many other sockets-related improvements.fixes #3229