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

Non-overriding signal handlers #2767

Closed
13 changes: 7 additions & 6 deletions lib/puma/cluster.rb
Expand Up @@ -5,6 +5,7 @@
require 'puma/plugin'
require 'puma/cluster/worker_handle'
require 'puma/cluster/worker'
require 'puma/signal'

require 'time'

Expand Down Expand Up @@ -288,7 +289,7 @@ def fork_worker!
# of the signals handlers as small as possible.
def setup_signals
if @options[:fork_worker]
Signal.trap "SIGURG" do
Puma::Signal.trap "SIGURG" do
fork_worker!
end

Expand All @@ -302,23 +303,23 @@ def setup_signals
end
end

Signal.trap "SIGCHLD" do
Puma::Signal.trap "SIGCHLD" do
wakeup!
end

Signal.trap "TTIN" do
Puma::Signal.trap "TTIN" do
@options[:workers] += 1
wakeup!
end

Signal.trap "TTOU" do
Puma::Signal.trap "TTOU" do
@options[:workers] -= 1 if @options[:workers] >= 2
wakeup!
end

master_pid = Process.pid

Signal.trap "SIGTERM" do
Puma::Signal.trap "SIGTERM" do
# The worker installs their own SIGTERM when booted.
# Until then, this is run by the worker and the worker
# should just exit if they get it.
Expand Down Expand Up @@ -418,7 +419,7 @@ def run

spawn_workers

Signal.trap "SIGINT" do
Puma::Signal.trap "SIGINT" do
stop
end

Expand Down
11 changes: 6 additions & 5 deletions lib/puma/cluster/worker.rb
Expand Up @@ -30,8 +30,9 @@ def run
title += " [#{@options[:tag]}]" if @options[:tag] && !@options[:tag].empty?
$0 = title

Signal.trap "SIGINT", "IGNORE"
Signal.trap "SIGCHLD", "DEFAULT"
Puma::Signal.trap "SIGINT", "IGNORE"

Puma::Signal.trap "SIGCHLD", "DEFAULT"

Thread.new do
Puma.set_thread_name "wrkr check"
Expand All @@ -55,7 +56,7 @@ def run
@launcher.config.run_hooks(:before_worker_boot, index, @launcher.log_writer)

begin
server = @server ||= start_server
server = @server ||= start_server
rescue Exception => e
log "! Unable to start worker"
log e.backtrace[0]
Expand All @@ -69,7 +70,7 @@ def run
if fork_worker
restart_server.clear
worker_pids = []
Signal.trap "SIGCHLD" do
Puma::Signal.trap "SIGCHLD" do
wakeup! if worker_pids.reject! do |p|
Process.wait(p, Process::WNOHANG) rescue true
end
Expand All @@ -96,7 +97,7 @@ def run
end
end

Signal.trap "SIGTERM" do
Puma::Signal.trap "SIGTERM" do
@worker_write << "e#{Process.pid}\n" rescue nil
restart_server.clear
server.stop
Expand Down
13 changes: 7 additions & 6 deletions lib/puma/launcher.rb
Expand Up @@ -7,6 +7,7 @@
require 'puma/single'
require 'puma/const'
require 'puma/binder'
require 'puma/signal'

module Puma
# Puma::Launcher is the single entry point for starting a Puma server based on user
Expand Down Expand Up @@ -415,7 +416,7 @@ def generate_restart_data

def setup_signals
begin
Signal.trap "SIGUSR2" do
Puma::Signal.trap "SIGUSR2" do
restart
end
rescue Exception
Expand All @@ -424,7 +425,7 @@ def setup_signals

unless Puma.jruby?
begin
Signal.trap "SIGUSR1" do
Puma::Signal.trap "SIGUSR1" do
phased_restart
end
rescue Exception
Expand All @@ -433,7 +434,7 @@ def setup_signals
end

begin
Signal.trap "SIGTERM" do
Puma::Signal.trap "SIGTERM" do
# Shortcut the control flow in case raise_exception_on_sigterm is true
do_graceful_stop

Expand All @@ -444,15 +445,15 @@ def setup_signals
end

begin
Signal.trap "SIGINT" do
Puma::Signal.trap "SIGINT" do
stop
end
rescue Exception
log "*** SIGINT not implemented, signal based gracefully stopping unavailable!"
Copy link
Contributor

Choose a reason for hiding this comment

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

This exception handling could also be moved to Puma::Signal module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, I'll consider it if the solution makes sense at all 😄

end

begin
Signal.trap "SIGHUP" do
Puma::Signal.trap "SIGHUP" do
if @runner.redirected_io?
@runner.redirect_io
else
Expand All @@ -465,7 +466,7 @@ def setup_signals

begin
unless Puma.jruby? # INFO in use by JVM already
Signal.trap "SIGINFO" do
Puma::Signal.trap "SIGINFO" do
thread_status do |name, backtrace|
@log_writer.log(name)
@log_writer.log(backtrace.map { |bt| " #{bt}" })
Expand Down
51 changes: 51 additions & 0 deletions lib/puma/signal.rb
@@ -0,0 +1,51 @@
module Puma
module Signal
module_function

def prepend_handler(sig, &handler)
name = signame(sig)
custom_signal_handlers[signame] ||= []
custom_signal_handlers[signame].prepend(handler)
end
nateberkopec marked this conversation as resolved.
Show resolved Hide resolved

# Signal.trap that does not replace
# the existing puma handlers
def trap(sig, handler_str=nil, &handler)
name = signame(sig)
::Signal.trap(name) do
invoke_custom_signal_handlers(name)

if handler.respond_to?(:call)
yield handler
else
# dirty hack to call string handlers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I'm stuck. First of all, I don't like this dirty hack. Besides that, it goes into an infinity loop on the SIGCHLD, I don't know exactly why.

@MSP-Greg could I use your assistance?

Copy link
Member

Choose a reason for hiding this comment

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

@alexeevit I'll have a look a bit later. Interesting failures, may have nothing to do with this PR. Not.sure.

Could you rebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👌

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Same problem as before... Sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? You thought the rebase could help?

Copy link
Contributor Author

@alexeevit alexeevit May 15, 2022

Choose a reason for hiding this comment

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

def custom_trap(sig, handler_str=nil, &handler)
  new_handler = proc do
    if handler.respond_to?(:call)
      yield handler
    else
      # dirty hack to call string handlers
      current_handler = Signal.trap(sig, handler_str)
      Process.kill(sig, Process.pid)
      Signal.trap(sig, new_handler) # or current_handler, same result
    end
  end

  Signal.trap(sig, new_handler)
end

custom_trap 'CHLD', 'IGNORE'

Process.kill('CHLD', Process.pid)

In ruby 2.5 it just exits and in ruby 2.6 it goes into an infinite loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, I'd like to have a better way to invoke the string handlers for signals (e.g. DEFAULT, SYSTEM_DEFAULT, IGNORE, ...). But this behavior looks quite interesting.

Copy link
Member

Choose a reason for hiding this comment

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

We might have the explanation for the problems seen here with Ruby 2.6+, see #3313 (comment)

Copy link
Contributor

@stanhu stanhu Jan 15, 2024

Choose a reason for hiding this comment

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

I tested the script in #2767 (comment) with Ruby 3.3, and it exited right away. This seems to suggest that the SIGCHLD handler introduced in Ruby 2.6 via ruby/ruby@054a412 may indeed be the issue.

Note that I tested the script with the ruby_3_2 branch, and it still got stuck. This is a different bug than the one described in #3313, which is actually a Ruby bug already fixed in the ruby_3_2 branch: https://bugs.ruby-lang.org/issues/19837

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stanhu thanks for the info!
However, it still isn't clear, what's the expected fix here. Should we support the feature only for the ruby versions that have the bug fixed? Didn't dig into the nature of the bug, so not sure if there is a workaround for other ruby versions.

# especially for DEFAULT and SYSTEM_DEFAULT
current_trap = ::Signal.trap(name, handler_str)
Process.kill(name, Process.pid)
::Signal.trap(name, current_trap)
end
end
end

private

def signame(sig)
name = sig.is_a?(Integer) ? ::Signal.signame(sig) : String(sig).sub('SIG', '')
raise ArgumentError, "unsupported signal SIG#{name}" unless ::Signal.list[name]
name
end
module_function :signame

def invoke_custom_signal_handlers(signame)
Array(custom_signal_handlers[signame]).each do |handler|
yield handler
end
end
module_function :invoke_custom_signal_handlers

def custom_signal_handlers
@custom_signal_handlers ||= {}
end
module_function :custom_signal_handlers
end
end