Remove dependencies on Thin, so Dashing can run on JRuby. #427
Conversation
|
Relates to #417, obviously :-) |
|
I now have a JRuby Dashboard running with this modified version of Dashing, released as dashing-jruby: http://jruby-dashboard.herokuapp.com/sample Sure would be nice to make it standard dashing :-D |
|
Tried installing dashing-jruby and replacing in my Gemfile and config.ru, but I get the following error when running "dashing start": /Library/Ruby/Gems/2.0.0/gems/bundler-1.6.4/lib/bundler/rubygems_integration.rb:256:in Am I doing something wrong? |
|
@Kevster Ahh "dashing start" must be trying to start thin directly. Will check. |
|
Ahh yes, dashing start/stop require that the server be able to daemonize, which can't be done through usual Ruby mechanisms on JRuby. I'm looking at doing something with nohup instead. |
|
I've pushed an additional change that removes "stop" and makes "start" use puma. Because there's no daemonizing on JRuby, start just launches the server and waits for it. I did not rig up something with nohup since it wouldn't work on e.g. Windows (not sure if that's a concern, since I don't think thin/em were reliable on Windows). I'm pushing dashing-jruby 1.3.4.1 with this change. |
|
Great, works like a charm! By the way, noticed you're using heroku. Did you specify a Procfile for using dashing-jruby (and puma)? |
|
I didn't do anything special. The https://github.com/jruby/jruby-dashboard repository is everything. |
| @@ -1,7 +1,7 @@ | |||
| # -*- encoding: utf-8 -*- | |||
|
|
|||
| Gem::Specification.new do |s| | |||
| s.name = 'dashing' | |||
| s.name = 'dashing-jruby' | |||
There was a problem hiding this comment.
There is no sense on put that line on the commit of your PR.
Because if it would merged, you would change the gemspec of the entire project to dashing-jruby.
I think you just want to fork that project, and on your fork only, put these line.
There was a problem hiding this comment.
Agreed, the project should remain available as the gem dashing.
However, the remainder of this PR has merit and could be considered. I have no allegiances to Thin.
There was a problem hiding this comment.
@cefigueiredo I opened this PR as a good faith attempt to work with you to make Dashing work on JRuby. I assumed the PR will evolve as we discuss its changes and find the best way to replace Thin with Puma. If you prefer to only receive fully-formed PRs before discussing them, I will try to do that in the future.
There was a problem hiding this comment.
That certainly is not what I am saying... My intent in commenting was to re-spark discussion around this. There are other issues that may be resolved by moving to Puma, so I think this is good even without providing jruby compatibility.
But the gem isn't going to be renamed... People are going to type gem install dashing 👍
|
👍 - it would be great to switch to puma for JRuby support |
| alias_method :stop_without_connection_closing, :stop | ||
| alias_method :stop, :stop_with_connection_closing | ||
| end | ||
|
|
There was a problem hiding this comment.
What is this code doing? Is there a Puma equivalent?
There was a problem hiding this comment.
I was not really sure about this :-\ It appears to monkey-patch Thin to close all open connections during shutdown. I don't think there needs to be an equivalent for Puma, because it already manages those connections and has a graceful shutdown path. Perhaps Thin would refuse to shut down unless all connections were closed?
There was a problem hiding this comment.
Yes, this seems weird. Maybe this is okay to remove...
I feel like Thin should already be managing those connections with a graceful shutdown path too...
There was a problem hiding this comment.
Here's the PR that introduced that change for some context: #383
I am aware that this is thing dependant, and I do want to take it out.
|
Sorry that I missed the replies in November...busy time for JRuby. I've commented on the source comments you provided and I'm looking into ways to spawn puma rather than starting it as a blocking child process. I think the best option will be to use nohup and puma's management interface to spawn and manage the subprocess, but I don't know what the equivalent would be on Windows. I'm also not sure how important Windows support is, since it appears there hasn't been a Windows release of EventMachine since 2013. |
|
Currently, Dashing can be run as a daemon with It seems that Puma uses a different model, so I think pumactl needs to be used like this: |
|
Right that is where I am at now. Demonization is complicated with JRuby because we can't fork, but the nohup approach plus state file should work well. |
|
Well here is my awful workaround to complete the switch to puma ... As my ubuntu server is a VM, i just to go the console and kick off |
|
Not sure how everyone else feels about this PR, but I think it'd be a positive move once the tests are fixed up to reflect this change. |
|
There are a number of requests for this in #183 -- seems like this should be merged after being fixed up. |
|
I want to make this happen while remaining backwards compatible |
|
I am going to take this over and get it up to code so we can merge it in, unless there are objections. |
|
Yup! Go for it man! +1000 |
|
Sounds great! |
Here's a quick pass to remove the dependency on Thin (and EventMachine) in favor of Puma. This allows dashing to work on JRuby, and Puma is a great server.
I was confused about the connection-closing block that used Thin-specific APIs. We can discuss what that code is doing and how to do it under Puma.