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

EventMachine -> concurrent-ruby, take two #23305

Merged
merged 8 commits into from Jan 29, 2016

Conversation

matthewd
Copy link
Member

No description provided.

@dhh
Copy link
Member

dhh commented Jan 29, 2016

Tested with Basecamp 👍

@dhh
Copy link
Member

dhh commented Jan 29, 2016

So feel free to merge.

matthewd added a commit that referenced this pull request Jan 29, 2016
EventMachine -> concurrent-ruby, take two
@matthewd matthewd merged commit 703ddad into rails:master Jan 29, 2016
@HoneyryderChuck
Copy link

@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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@matthewd
Copy link
Member Author

nio4r uses libev for MRI

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

@HoneyryderChuck
Copy link

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

@matthewd
Copy link
Member Author

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
Copy link

@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
Copy link

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
Copy link
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
Copy link

@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 concurrent-take-2 branch June 11, 2017 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants