Implement Addrinfo.getaddrinfo #1377

Closed
sferik opened this Issue Jan 2, 2014 · 17 comments

4 participants

@sferik

If you look at Travis CI build 183 of the tarcieri/http repo, you’ll see that it passes on every major CRuby version (1.8.7 through 2.1.0), Rubinius 2, and JRuby 1.7.9 (in both Ruby 1.8 and Ruby 1.9 modes).

However, it fails on JRuby 9K. Here is the full version description:

jruby 9000.dev (2.1.0.dev) 2013-12-27 c66a441 on Java HotSpot(TM) 64-Bit Server VM 1.7.0_45-b18 [linux-amd64]

Specifically, the test suite errors out whilst starting WEBrick, raising the following exception. I have snipped just the relevant portion of the stack trace:

NoMethodError: undefined method `each' for nil:NilClass
             foreach at /home/travis/.rvm/rubies/jruby-head/lib/ruby/2.1/socket.rb:217
  tcp_server_sockets at /home/travis/.rvm/rubies/jruby-head/lib/ruby/2.1/socket.rb:441
    create_listeners at /home/travis/.rvm/rubies/jruby-head/lib/ruby/2.1/webrick/utils.rb:75
              listen at /home/travis/.rvm/rubies/jruby-head/lib/ruby/2.1/webrick/server.rb:132
          initialize at /home/travis/.rvm/rubies/jruby-head/lib/ruby/2.1/webrick/server.rb:113
          initialize at /home/travis/.rvm/rubies/jruby-head/lib/ruby/2.1/webrick/httpserver.rb:45
              (root) at /home/travis/build/tarcieri/http/spec/support/example_server.rb:75

The error originates with this seemingly innocuous line of code:

ExampleServer = WEBrick::HTTPServer.new(:Port => ExampleService::PORT, :AccessLog => [])

Here is the full source code of that file at the current snapshot in time.

Before reporting this issue, I chatted with @enebo on IRC. He suggested that this failure was related to the fact that Addrinfo::getaddrinfo is not yet implemented.

public static IRubyObject getaddrinfo(ThreadContext context, IRubyObject recv, IRubyObject[] args) {
    // unimplemented
    return context.nil;
}

According to @enebo:

no doubt someone can impl this but it is 666 lines of code in MRI for getaddrinfo…looks like although Java net APIs cannot do this stuff, it can be done through jnr-posix and friends or perhaps even an FFI impl

Unfortunately, I am highly unqualified to even begin working on this myself but I wish you the best of luck with the implementation. 🙏🍀


The rspec command also outputs the following warnings, which seem to be unrelated, but I thought would be worth reporting anyway:

/home/travis/.rvm/rubies/jruby-head/lib/ruby/shared/krypt/hmac.rb:54 warning: shadowing outer local variable - new_key
/home/travis/.rvm/rubies/jruby-head/lib/ruby/shared/krypt/codec/hex.rb:65 warning: shadowing outer local variable - data
/home/travis/.rvm/rubies/jruby-head/lib/ruby/shared/krypt/codec/hex.rb:112 warning: shadowing outer local variable - data
/home/travis/.rvm/rubies/jruby-head/lib/ruby/shared/krypt/codec/base64.rb:78 warning: shadowing outer local variable - data
/home/travis/.rvm/rubies/jruby-head/lib/ruby/shared/krypt/codec/base64.rb:130 warning: shadowing outer local variable - data

These warnings also occur on JRuby 1.7.9 in Ruby 1.9 mode, where the build is passing, which is why I believe they are unrelated. Note: they do not occur on JRuby 1.7.9 in Ruby 1.8 mode.

/cc @tarcieri

@sferik

At the suggestion of @whit537 and @mbj, I’ve placed a $150 bounty on the resolution of this issue. I know that amount is pales in comparison to the amount of time and effort it will take to resolve this issue but I figured it was better than nothing. If others would like to see this issue resolved, they may also pledge money to increase the amount at Bountysource.

This is my first time backing an issue on Bountysource, so I consider it a bit of an experiment. I’d be curious to get feedback on how you perceive this, how/whether it affects your motivation, and how/whether it causes this issue to be prioritized differently than it would have otherwise.

The bounty expires in 6 months, on July 2, 2013.

@tarcieri

$50 added to this bounty 😺

@sferik

💥

@robertoandrade

@sferik you meant on July 2, 2014 right? :)

@sferik

@robertoandrade Yes, 2014. Happy New Year! 🎊

@headius
JRuby Team member

A correction of the original report... it is Addrinfo.getaddrinfo that's unimplemented, not Socket.getaddrinfo. The latter does have at least a partial implementation.

@sferik

@headius I have update the title of the issue to reflect this correction. Please note that the bounty applies to what I meant, not to what I said. 😉

@headius
JRuby Team member

I have implemented more Addrinfo logic in 3c1ce22 and servers now appear to start up correctly. There are a few caveats:

  • I only implemented what was necessary to get test_http passing most of its tests, since that seemed like the most relevant test. We are not yet running this test since it appears to need root access, but we could add it to our suite if a workaround can be found.
  • I modified a few lines in socket.rb to construct ServerSocket instead of Socket, since because of JDK's API design we can't do servers with Socket (only clients). One of these lines, https://github.com/jruby/jruby/blob/3c1ce2278272ab10840f27d412e6e48014efdb8a/lib/ruby/2.1/socket.rb#L372, may not be appropriate to hardcode to ServerSocket. I have not unraveled all this new socket.rb logic yet to be sure.

I'm not sure if this meets the requirements of the bounty, but I believe it should now work correctly for server initialization. Surprisingly, after these few changes, test_http passed all but four tests that appear to be unrelated to actually starting up the server, where 42 tests failed or errored out before.

I have not run tarcieri/http tests yet.

@headius
JRuby Team member

The root-run issue appears to be a separate problem with BasicSocket#for_fd; WEBrick's utils.rb:78 converts the sockets from Socket instances to TCPServer instances via for_fd, which we don't implement right.

Unable to confirm this is fixed until that other issue is resolved, so I'm working on that now.

@headius
JRuby Team member

Side note: The new logic for net/http and webrick are much more complicated than in the past and make heavy use of low-level APIs we can only simulate. This is a problem.

@headius
JRuby Team member

With fixes in b403d67 I can now run test_http without root, so I think this is close to being fixed. Running the tarcieri/http tests seems to produce a Errno::EADDRINUSE: Address already in use - bind - Address already in use error, however, and I've not yet investigated where that comes from.

@sferik

Running the tarcieri/http tests seems to produce a Errno::EADDRINUSE: Address already in use - bind - Address already in use error, however, and I've not yet investigated where that comes from.

Does the following command produce any output?

sudo lsof -ti :65432

If so, that’s the process that’s already bound to the port WEBrick is trying to use.

@sferik

If you’d like another codebase to test against, I’m also seeing the build fail with the same error in RailsAdmin.

@headius
JRuby Team member

Re-ran the specs today and things look ok. I may have had a rogue process in the background.

https://gist.github.com/headius/8308530

So, let's call this fixed. I'll check out RailsAdmin separately.

@headius headius closed this Jan 7, 2014
@headius
JRuby Team member

I claimed the bounty. The money will go into JRuby funds.

I have also merged the fixes to jruby-1_7, since the Addrinfo feature was available in 1.9.3.

@sferik

@headius Awesome! I just added a definition for JRuby 1.7.10 to ruby-build and pushed a new release to homebrew.

Thanks again for fixing this so quickly!

@headius
JRuby Team member

FWIW, the Addrinfo stuff did not get into 1.7.10, but it will be in 1.7.11 in the next few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment