stop puma from sending INT on SIGHUP which we use for logrotate #2637
Conversation
grosser
force-pushed
the
grosser/sighup
branch
2 times, most recently
from
132d483
to
00c1b23
Compare
lib/samson/logging.rb
Outdated
| # The rails logger does not know what file it opened, so we have to help it https://github.com/rails/rails/issues/32211 | ||
| # If we do not have a file-based logger we still do not want to kill the process, but log that we got the signal. | ||
| # We need to use self-pipe since we cannot reopen logs in an interrupt. | ||
| # This will break stdout_redirect feature of puma, which I hope nobody uses. |
lib/samson/logging.rb
Outdated
| # This will break stdout_redirect feature of puma, which I hope nobody uses. | ||
| if ENV["SERVER_MODE"] | ||
| read, write = IO.pipe | ||
| Signal.trap(:SIGHUP) { write.puts } |
There was a problem hiding this comment.
Doesn't this swallow SIGHUP entirely? Don't we need to propagate the signal somehow?
There was a problem hiding this comment.
swallowing is exactly what we want since puma translates HUP -> INT
lib/samson/logging.rb
Outdated
| Rails.logger.info "Received SIGHUP ... reopening logs" | ||
| dev = Rails.logger.instance_variable_get(:@logdev)&.dev | ||
| path = dev.path if dev&.is_a?(File) | ||
| Rails.logger.reopen(path) |
There was a problem hiding this comment.
We want to call Rails.logger.reopen(nil) if dev isn't a file?
There was a problem hiding this comment.
Calling with nil is a noop for all the other loggers, so not calling it is fine too ... maybe more obvious what is going on by not calling it
grosser
force-pushed
the
grosser/sighup
branch
2 times, most recently
from
5f4aca5
to
dfdcbf6
Compare
|
reworked to make our hack more obvious since we don't need all this file reopen logic anyway ... looks good ? |
lib/samson/logging.rb
Outdated
| Rails.logger.info "Received SIGHUP ... reopening logs" | ||
| # Rails.logger does not know what file it opened, so we help it https://github.com/rails/rails/issues/32211 | ||
| dev = Rails.logger.instance_variable_get(:@logdev)&.dev | ||
| Rails.logger.reopen(dev.path) |
There was a problem hiding this comment.
Isn't it possible for dev to be nil here, so you'd be calling nil.path?
There was a problem hiding this comment.
no since this must be a file logger at that point ... I'll remove the &. too that was ;eftover from previous approach
rails/rails#32211
/cc @ragurney @jonmoter