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

EventMachine -> concurrent-ruby, take two #23305

Merged
merged 8 commits into from Jan 29, 2016

Conversation

Projects
None yet
6 participants
@matthewd
Member

matthewd commented Jan 28, 2016

No description provided.

@dhh

This comment has been minimized.

Member

dhh commented Jan 29, 2016

Tested with Basecamp 👍

@dhh

This comment has been minimized.

Member

dhh commented Jan 29, 2016

So feel free to merge.

matthewd added some commits Jan 24, 2016

Synchronize the lazy setters in Server
They're all at risk of races on the first requests.
Handle more IO errors (especially, ECONNRESET)
Also, address the possibility of the listen thread dying and needing to
be respawned. As a bonus, we now defer construction of the thread until
we are first given something to monitor.
Keep the socket reference after close
We may still try to send to it.

@matthewd matthewd force-pushed the matthewd:concurrent-take-2 branch to 4d01cd1 Jan 29, 2016

matthewd added a commit that referenced this pull request Jan 29, 2016

Merge pull request #23305 from matthewd/concurrent-take-2
EventMachine -> concurrent-ruby, take two

@matthewd matthewd merged commit 703ddad into rails:master Jan 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@HoneyryderChuck

This comment has been minimized.

HoneyryderChuck commented Jan 30, 2016

@matthewd nio4r uses libev for MRI. If the purpose of removing eventmachine was to have support for windows MRI, that won't do it, at least until someone port nio4r to libuv.

def self.secure_request?(env)
return true if env['HTTPS'] == 'on'
return true if env['HTTP_X_FORWARDED_SSL'] == 'on'

This comment has been minimized.

@ianks

ianks Jan 30, 2016

Contributor

Might be wise to use Rack::Reqest#ssl? here.

This comment has been minimized.

@exAspArk

exAspArk Feb 1, 2016

Contributor

Or maybe it's better to use Rack::Reqest#scheme == 'https' then?

@@ -29,25 +36,31 @@ def disconnect(identifiers)
# Gateway to RemoteConnections. See that class for details.
def remote_connections
@remote_connections ||= RemoteConnections.new(self)
@remote_connections || @mutex.synchronize { @remote_connections ||= RemoteConnections.new(self) }

This comment has been minimized.

@ianks

ianks Jan 30, 2016

Contributor

Why not use AtomicReferences for these?

@matthewd

This comment has been minimized.

Member

matthewd commented Jan 30, 2016

nio4r uses libev for MRI

@TiagoCardoso1983 except on Windows, where it falls back to Kernel#select.

@matthewd matthewd referenced this pull request Jan 31, 2016

Merged

Redis sans EventMachine #23381

@42thcoder 42thcoder referenced this pull request Feb 1, 2016

Closed

一日见闻 [2016.2.1] #3

@HoneyryderChuck

This comment has been minimized.

HoneyryderChuck commented Feb 4, 2016

@matthewd what about socketry/nio4r#52? nio4r pure ruby implementation is quite untested and contains recursive locking issues.

@matthewd

This comment has been minimized.

Member

matthewd commented Feb 4, 2016

@TiagoCardoso1983 we're not using the block form of select, so that bug doesn't affect us. But even if it did, the existence of bugs in a library doesn't seem particularly shocking to me. They will exist, and they will hopefully be fixed.

I'm not sure what you're implying we should do differently here. Do you have a specific recommendation?

@HoneyryderChuck

This comment has been minimized.

HoneyryderChuck commented Feb 4, 2016

@matthewd the main reason eventmachine was removed is because it doesn't build under windows, right? I do prefer the implementation using nio4r, just wanted to state the windows potential issue while rails is still in beta mode so anyone with access to the environment could test it. Rails historically refrains from using dependencies with extensions so that it can "go where ruby goes". This contract has been broken once already with the html sanitizer gem. I'm afraid most of the tester pool will be using some *NIX form running against the libev reactor and will not test the fallback IO.select, so just wanted to state the potential drawback before the official release of 5.

@schmurfy

This comment has been minimized.

schmurfy commented Feb 4, 2016

seriously is there really anyone running production rails code under windows ? For development I can understand but I would be impressed otherwise. Eventmachine also has had bugs of its own for a while now althought development seems to now happens again on it after years of a mostly abandonned codebase with a laundry of pull requests ready to be merged with nobody to do it.

concurrent-ruby and nio4r seems like the best bet for me and a really good choice.

@dhh

This comment has been minimized.

Member

dhh commented Feb 4, 2016

I'm not concerned about Windows production deployments either. The main use
case is making sure it's OK for development. What we have now works for
that. If someone finds bugs during Windows development, we can treat them
as any other type of bug.

On Thu, Feb 4, 2016 at 4:05 PM, Julien Ammous notifications@github.com
wrote:

seriously is there really anyone running production rails code under
windows ? For development I can understand but I would be impressed
otherwise. Eventmachine also has had bugs of its own for a while now
althought development seems to now happens again on it after years of a
mostly abandonned codebase with a laundry of pull requests ready to be
merged with nobody to do it.

concurrent-ruby and nio4r seems like the best bet for me and a really good
choice.


Reply to this email directly or view it on GitHub
#23305 (comment).

@HoneyryderChuck

This comment has been minimized.

HoneyryderChuck commented Feb 4, 2016

@schmurfy I don't think anyone wants to deploy action cable in production relying on IO.select, whatever the platform is :) Windows is still a relevant development environment, and is annoying mostly because a lot of dependencies go the "we only support UNIX" way.

Anyway, I can see this going the activejob/rails way in that one supports the basic setup for synch-jobs/webrick and then it's up for the developer to decide the production setup and dependencies. Main requirement for the basic setup is that it works well where ruby works.

@matthewd matthewd deleted the matthewd:concurrent-take-2 branch Jun 11, 2017

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