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

Switch IO reactor to nio4r #1728

Merged
merged 9 commits into from Mar 20, 2019

Conversation

Projects
None yet
6 participants
@evanphx
Copy link
Member

commented Feb 20, 2019

This moves away from IO.select to using nio4r to allow the reactor to
scale beyond 1024 active clients. This happens when folks are using
websockets usually.

Switch IO reactor to nio4r
This moves away from IO.select to using nio4r to allow the reactor to
scale beyond 1024 active clients. This happens when folks are using
websockets usually.

@evanphx evanphx requested review from schneems and nateberkopec Feb 20, 2019

@MSP-Greg

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

@evanphx

nio4r uses libev, which isn't Windows compatible (would need to change to libuv). Thoughts? I think Appveyor will probably break with this.

@evanphx

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

@MSP-Greg Happy to consider other options, for sure. On Windows it will fall back to IO.select I believe, which means Windows would stay as-is. Would be nice to allow Windows to take advantage of this change, but it's net neutral otherwise.

@MSP-Greg

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

On Windows it will fall back to IO.select I believe

Sorry, I should have looked at it more. Passed on Appveyor.

nio4r is aware of the issue, but they thought switching to libuv is (understandably) something they're not interested in changing at this time. I'm not a c type, so...

Show resolved Hide resolved lib/puma/reactor.rb Outdated
@@ -182,7 +204,8 @@ def run_internal
c.write_500
c.close

sockets.delete c
selector.deregister m

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Feb 20, 2019

Member

maybe this could be it's own method

@HoneyryderChuck

This comment has been minimized.

Copy link

commented Feb 20, 2019

You can avoid allocating an array each time you call #select by passing a block to select.

From https://github.com/socketry/nio4r/wiki/Selectors#selecting-io-objects-for-readiness , if it's a nice-to-have.

@schneems
Copy link
Contributor

left a comment

Overall lit seems reasonable. Looks like mostly a re-mapping of the behavior that was there before. I'll have to update all the docs on this class to not be based on select() once it's merged.

For this:

# There is a bug where we seem to be registering an already registered
# client. This code deals with this situation but I wish we didn't have to.

Is there a test that fails if you remove this code or is it an intermittent failure? Basically, I want to make sure that we know where to start looking if we decide we want to debug this in the future.

The other comment about switching to use a block instead of allocating an array would be a nice perf bump, but we can also leave that as a TODO.

Looks like there's still a failing test on 2.5.3 for now:

  1) Error:
TestPersistent#test_post_then_get:
TypeError: no implicit conversion of nil into String
    /Users/travis/build/puma/puma/test/test_persistent.rb:42:in `block (2 levels) in lines'
    /Users/travis/build/puma/puma/test/test_persistent.rb:42:in `times'
    /Users/travis/build/puma/puma/test/test_persistent.rb:42:in `block in lines'
    /Users/travis/.rvm/rubies/ruby-2.5.3/lib/ruby/2.5.0/timeout.rb:93:in `block in timeout'
    /Users/travis/.rvm/rubies/ruby-2.5.3/lib/ruby/2.5.0/timeout.rb:33:in `block in catch'
    /Users/travis/.rvm/rubies/ruby-2.5.3/lib/ruby/2.5.0/timeout.rb:33:in `catch'
    /Users/travis/.rvm/rubies/ruby-2.5.3/lib/ruby/2.5.0/timeout.rb:33:in `catch'
    /Users/travis/.rvm/rubies/ruby-2.5.3/lib/ruby/2.5.0/timeout.rb:108:in `timeout'
    /Users/travis/build/puma/puma/test/test_persistent.rb:41:in `lines'
    /Users/travis/build/puma/puma/test/test_persistent.rb:79:in `test_post_then_get'

And a few failing against jruby and windows.

rescue IOError
# Means that the io is closed, so we should ignore this request
# entirely
else

This comment has been minimized.

Copy link
@schneems

schneems Feb 20, 2019

Contributor

This threw me off, wanted to mention in case anyone else didn't see what was happening. This else only gets fired if there's no exception https://blog.newrelic.com/engineering/weird-ruby-2-rescue-interrupt-ensure/

This comment has been minimized.

Copy link
@evanphx

evanphx Feb 21, 2019

Author Member

One of my fav underused constructs!

@MSP-Greg

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

The Appveyor build after the rebase is a false positive. Apologies, I never allowed for gem installation to fail. It should work now...

evanphx added some commits Feb 21, 2019

@evanphx evanphx requested review from schneems and nateberkopec Mar 20, 2019

@schneems
Copy link
Contributor

left a comment

@evanphx evanphx merged commit 1bda762 into master Mar 20, 2019

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ioquatix

This comment has been minimized.

Copy link

commented Jun 8, 2019

As the maintainer of nio4r, I'd be happy to evaluate other options. Or even build something entirely new on top of libuv. I don't think there has been any decision like we shouldn't rebuild nio4r on top of libuv just FYI. The surface area of libuv is huge compared to nio4r.

I think nio4r is fine for what it is. I don't think it will still be useful in 10 years. Things are moving rapidly in this space, e.g. liburing.

The actual problem space is pretty large though. For example, nio4r is primarily concerned with exposing non-blocking network sockets, but there are many areas where blocking behaviour still exists: disk IO (e.g. read, write, readdir), database IO (e.g. libpq), name resolution (e.g. DNS) and a whole ton of other stuff.

If your only issue is blocking network IO, then nio4r is fine and a solid solution. It would be nice to make sure it is as good as possible, e.g. on Windows too, but honestly that's not a huge priority for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.