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

Fix rails restart issue with Puma #24331

Merged
merged 1 commit into from Mar 30, 2016

Conversation

Projects
None yet
6 participants
@prathamesh-sonpatki
Member

prathamesh-sonpatki commented Mar 26, 2016

  • We need to pass the restart command to Puma so that it will use it
    while restarting the server.
  • Besides that we need to remove the pid file for the previous running
    server because otherwise Rack complains about it's presence.
  • This also requires some changes on Puma side which are being tracked
    here - puma/puma#936.
  • Fixes #23910.

r? @rafaelfranca cc @evanphx

@rubys

View changes

railties/lib/rails/commands/server.rb Outdated
@@ -94,7 +94,8 @@ def default_options
environment: (ENV['RAILS_ENV'] || ENV['RACK_ENV'] || "development").dup,
daemonize: false,
caching: nil,
pid: Options::DEFAULT_PID_PATH
pid: Options::DEFAULT_PID_PATH,
restart_cmd: 'bin/rails s'

This comment has been minimized.

@rubys

rubys Mar 27, 2016

Contributor

Question: if I start the rails server using, say:

rails server -b 0.0.0.0 -p 4567

And then restart the server using:

rails touch

Will the same options be used?

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Mar 27, 2016

Member

@rubys No. It will start on port 3000 instead of 4567.

$ rails server -b 0.0.0.0 -p 4567
=> Booting Puma
=> Rails 5.0.0.beta3 application starting in development on http://0.0.0.0:4567
=> Run `rails server -h` for more startup options
Puma starting in single mode...
* Version 3.2.0 (ruby 2.3.0-p0), codename: Spring Is A Heliocentric Viewpoint
* Min threads: 5, max threads: 5
* Environment: development
* Listening on tcp://0.0.0.0:4567
Use Ctrl-C to stop
* Restarting...
=> Booting Puma
=> Rails 5.0.0.beta3 application starting in development on http://localhost:3000
=> Run `rails server -h` for more startup options
Puma starting in single mode...
* Version 3.2.0 (ruby 2.3.0-p0), codename: Spring Is A Heliocentric Viewpoint
* Min threads: 5, max threads: 5
* Environment: development
* Listening on tcp://localhost:3000
* Closing unused inherited connection: tcp://0.0.0.0:4567
Use Ctrl-C to stop

I hadn't thought of this scenario. I will check how to make it work.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Mar 27, 2016

Member

@rubys I fixed it here - 2337989.

Now it works as expected.

$ rails server -b 0.0.0.0 -p 4567
=> Booting Puma
=> Rails 5.0.0.beta3 application starting in development on http://0.0.0.0:4567
=> Run `rails server -h` for more startup options
Puma starting in single mode...
* Version 3.2.0 (ruby 2.3.0-p0), codename: Spring Is A Heliocentric Viewpoint
* Min threads: 5, max threads: 5
* Environment: development
* Listening on tcp://0.0.0.0:4567
Use Ctrl-C to stop
* Restarting...
=> Booting Puma
=> Rails 5.0.0.beta3 application starting in development on http://0.0.0.0:4567
=> Run `rails server -h` for more startup options
Puma starting in single mode...
* Version 3.2.0 (ruby 2.3.0-p0), codename: Spring Is A Heliocentric Viewpoint
* Min threads: 5, max threads: 5
* Environment: development
* Inherited tcp://0.0.0.0:4567
Use Ctrl-C to stop
@@ -15,6 +15,7 @@ def enable_by_file
end
FileUtils.touch 'tmp/restart.txt'
FileUtils.rm_f('tmp/pids/server.pid')

This comment has been minimized.

@kaspth

kaspth Mar 27, 2016

Member

Let's extend the existing test which asserts that the temp file has been removed to also assert that the pid file is removed.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Mar 30, 2016

Member

Added coverage.

@@ -2,4 +2,5 @@ desc "Restart app by touching tmp/restart.txt"
task :restart do
FileUtils.mkdir_p('tmp')
FileUtils.touch('tmp/restart.txt')
FileUtils.rm_f('tmp/pids/server.pid')

This comment has been minimized.

@kaspth

kaspth Mar 27, 2016

Member

Ditto here.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Mar 30, 2016

Member

Added coverage.

@kaspth

View changes

railties/test/commands/server_test.rb Outdated
def test_restart_command_contains_customized_options
args = ["-p", "4567"]
ARGV.replace args

This comment has been minimized.

@kaspth

kaspth Mar 27, 2016

Member

Is this inline mutation a good idea? Wouldn't at least be better to capture the old ARGV value and reset in ensure.

This comment has been minimized.

@rafaelfranca
@kaspth

View changes

railties/lib/rails/commands/server.rb Outdated
@@ -130,5 +131,9 @@ def log_to_stdout
Rails.logger.extend(ActiveSupport::Logger.broadcast(console))
end
end
def restart_command
"bin/rails s #{ARGV.join(' ')}"

This comment has been minimized.

@kaspth

kaspth Mar 27, 2016

Member

Can we just expand the s to server? 😁

This comment has been minimized.

@rafaelfranca

This comment has been minimized.

@prathamesh-sonpatki
@@ -94,7 +94,8 @@ def default_options
environment: (ENV['RAILS_ENV'] || ENV['RACK_ENV'] || "development").dup,
daemonize: false,
caching: nil,
pid: Options::DEFAULT_PID_PATH
pid: Options::DEFAULT_PID_PATH,
restart_cmd: restart_command

This comment has been minimized.

@kaspth

kaspth Mar 27, 2016

Member

Is restart_cmd guaranteed to work for other servers people might use in dev?

This comment has been minimized.

@kaspth

kaspth Mar 27, 2016

Member

By "work" I mean that they won't blow up on unsupported keys for instance (not that they will automatically use the generated command).

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Mar 30, 2016

Member

I tested on Thin, Unicorn and Webrick. The restart_cmd has no effect on them. Meaning rails restart continues to not work while using those servers as before.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Mar 30, 2016

Member

Also these servers don't blow up on unsupported key restart_cmd.

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:fix-puma-restart branch Mar 30, 2016

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Mar 30, 2016

@kaspth Thanks for the review! I updated the code.

assert_equal expected, server.default_options[:restart_cmd]
ensure
ARGV.replace original_args

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Mar 30, 2016

Member

Made sure that original ARGV are replaced at the end of test. The other option was to use test specific constant TEST_ARGV like SPEC_ARGV in Puma tests but I preferred this option over that.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Mar 30, 2016

Member

The other option is to use ARGV[0..-1] = args instead of ARGV.replace.

@rafaelfranca What do you think about ARGV[0..-1] = args?

This comment has been minimized.

@rafaelfranca

rafaelfranca Mar 30, 2016

Member

replace is fine to me.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Mar 30, 2016

Tests are broken.

Fix rails restart issue with Puma
- We need to pass the restart command to Puma so that it will use it
  while restarting the server.
- Also made sure that all the options passed by user while starting
  the server are used in the generated restart command so that they will
  be used while restarting the server.
- Besides that we need to remove the server.pid file for the previous running
  server because otherwise Rack complains about it's presence.
- We don't care if the server.pid file does not exist. We only want to delete
  it if it exists.
- This also requires some changes on Puma side which are being tracked
  here - puma/puma#936.
- Fixes #23910.

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:fix-puma-restart branch to 9d87ce3 Mar 30, 2016

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Mar 30, 2016

Thanks, I fixed the tests.

@rafaelfranca rafaelfranca merged commit 6065f0e into rails:master Mar 30, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@prathamesh-sonpatki prathamesh-sonpatki deleted the prathamesh-sonpatki:fix-puma-restart branch Mar 30, 2016

@kaspth

This comment has been minimized.

Member

kaspth commented Mar 30, 2016

❤️🤘

@steakknife

This comment has been minimized.

Contributor

steakknife commented Oct 23, 2016

This should be backported to stable branches. It's a minimal change needed ASAP.

puma/puma#1117

EDIT: This fix doesn't address Puma 3.6.0 + Rails 5.0.0.1. (cc @rafaelfranca). An easy way to fix it would be for Rails to not set the :pid option by default when using Puma since it appears to manages it.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Oct 23, 2016

Which stable branch are you talking about? This is already in 5.0.0.

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Oct 23, 2016

I looked into this issue. With Puma 3.5.0 and above, I am getting this error, https://gist.github.com/prathamesh-sonpatki/bc0cac8929dbf17316e8b4b7dda93e20.

With Puma 3.4 and 3.3, the rails restart command is working as expected. I will look into why it stopped working with Puma 3.5.

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Oct 23, 2016

I have opened puma/puma#1120 to fix this issue for Pume 3.5 and above.

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