Do not generate pidfile in production environments #50644
Conversation
| @@ -243,11 +243,13 @@ def log_to_stdout? | |||
| end | |||
|
|
|||
| def pid | |||
| File.expand_path(options[:pid] || ENV.fetch("PIDFILE", DEFAULT_PIDFILE)) | |||
| default_pidfile = environment == "development" ? DEFAULT_PIDFILE : nil | |||
| pid = options[:pid] || ENV["PIDFILE"] || default_pidfile | |||
There was a problem hiding this comment.
If someone specify they PIDFILE env or the pid option we should generate it regardless of the environment.
There was a problem hiding this comment.
If someone specify they PIDFILE env or the pid option we should generate it regardless of the environment.
@rafaelfranca I'm sorry if I got it somehow wrong, but isn't that what happens? 😅
The assignment to pid is evaluated regardless of the environment. So if the user sets either the option or ENV["PIDFILE"], their respective values will be assigned to pid
pid = options[:pid] || ENV["PIDFILE"] || default_pidfile # < always non-nil if e.g. PIDFILE is assigned
Only the 'fallback' default_pidfile is unassigned in production, everything else stays the same as before. Open to suggestions on how to make the code clearer 🙂
There was a problem hiding this comment.
lol, right. Sorry. I got it wrong
Co-authored-by: Rafael Mendonça França <rafael@franca.dev>
| if ENV["PIDFILE"] | ||
| pidfile ENV["PIDFILE"] | ||
| else | ||
| pidfile tmp/pids/server.pid" if ENV.fetch("RAILS_ENV", "development") == "development" |
There was a problem hiding this comment.
@hschne There is missing "
- pidfile tmp/pids/server.pid" if ENV.fetch("RAILS_ENV", "development") == "development"
+ pidfile "tmp/pids/server.pid" if ENV.fetch("RAILS_ENV", "development") == "development"There was a problem hiding this comment.
Also what do you think about simplifying the if-condition to reflect to what we say in the changelog: " Disable pidfile generation in production environment."
unless ENV["RAILS_ENV"] == "production"
Would that work?
There was a problem hiding this comment.
Looks like pidfile generation in railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt was updated further by #50669.
@rafaelfranca Wondering whether we should sync
actionmailbox/test/dummy/config/puma.rbactiontext/test/dummy/config/puma.rbactivestorage/test/dummy/config/puma.rb
files with railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt? 🤔
Motivation / Background
This is a follow-up to #50628.
When running Rails applications with Docker containers may fail to restart if they crashed (e.g. because of OOM). This is because the
/rails/tmp/pids/server.pidfile is already present.Such failed containers will require manual intervention (e.g. remove the faulty container) to get up and running again. To allow containers to restart the server.pid file must be deleted on container start. As suggested in #50628, rather than removing the file in the
docker-entrypoint, the default for new Rails applications should be not to create PIDs in the first place in production.This Pull Request has been created to not create pidfiles in production.
Detail
This makes the
pidfileinstruction from the defaultpuma.tttemplate conditional. The puma files in the respective dummy / test apps have been updated for consistency. The default path for the pidfile in the server command has also been made conditional.Additional information
I opted for making pidfile generation conditional rather than removing it outright as it has its uses in development (e.g. when not running with Docker per default). Might be though that this conditional behaviour ends up causing some confusion. Thoughts?