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

Fix for #3229 #4152

Merged
merged 27 commits into from Sep 22, 2016
Merged

Fix for #3229 #4152

merged 27 commits into from Sep 22, 2016

Conversation

@etehtsea
Copy link
Contributor

@etehtsea etehtsea commented Sep 13, 2016

Almost full Addrinfo support and many other sockets-related improvements.

fixes #3229

headius and others added 26 commits Sep 12, 2016
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 {
Copy link
Member

@kares kares Sep 13, 2016

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> 

Copy link
Member

@headius headius Sep 13, 2016

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.

@headius
Copy link
Member

@headius headius commented Sep 13, 2016

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.

@headius
Copy link
Member

@headius headius commented Sep 13, 2016

@etehtsea It looks good to me. Hard to tell which code is yours and which is mine now :-)

The IPSocket abstract/NotAllocatable change should be reverted, but that's all I saw.

Maybe @enebo can weigh in from Japan if he's got a moment.

@headius headius added this to the JRuby 9.1.6.0 milestone Sep 13, 2016
@enebo
Copy link
Member

@enebo enebo commented Sep 14, 2016

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.

@etehtsea
Copy link
Contributor Author

@etehtsea etehtsea commented Sep 14, 2016

@kares, @headius done. Also reverted for BasicSocket:

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 context.runtime: I'll take this into account.

Make RubyBasicSocket and RubyIPSocket allocatable to match MRI behaviour
@headius headius merged commit cc619c0 into jruby:master Sep 22, 2016
1 of 2 checks passed
@etehtsea etehtsea deleted the socket-next-re branch Sep 22, 2016
@headius
Copy link
Member

@headius headius commented Sep 22, 2016

@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.

@eregon
Copy link
Member

@eregon eregon commented Nov 27, 2016

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants