Non-overriding signal handlers

[alexeevit]

Description

Closes #2701

I found that Puma overrides all custom signal handlers, e.g. those that are defined in Rails initializers. Here's the implementation of safe signal handlers that allows having custom signal handlers in applications.

BTW, I'm not sure what tests I should add here so I'd be happy to get some directions.

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[alexeevit]

Ok, I'm working to cover the Signal.trap(signal, command) case

[alexeevit]

@nateberkopec hey, I've got a question here.

First of all, the previous implementation (executing all previous traps before the latest one) is not our case. In Puma code, we override traps multiple times so we can't just turn off those overriding.

But that'd be cool to have a mechanism to prepend custom application signal handlers before the Puma's ones, e.g. for debugging purposes like in the initial issue #2767 was requested. (Puma::Signal.prepend_handler)

So here I created a small PoC how it could be done and I have done a few tests with a rails app and it kinda works.
But I'm really not sure that we need this type of solution for such a problem.

I let you decide now.

[johnnyshields]

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

[alexeevit]

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

[nateberkopec]

So I guess the idea is that if I'm a Rails app developer, I'll have to use Puma::Signal.trap to set up a signal in config/initializers or whatever to setup a signal that I don't want overridden?

[mperham]

Someone recently submitted a similar type of patch so that Sidekiq's signal handler calls any previously registered trap:

https://github.com/mperham/sidekiq/blob/4ce86cda9e968de0d3aa198839c2dd0ac4c4f17b/lib/sidekiq/cli.rb#L48-L62

[nateberkopec]

Also pointed out that Minitest has some code for this

[nateberkopec]

Maybe app developers should just be using something like this themselves. Since Puma itself doesn't need this functionality, and we can't do anything about people defining signal traps in their Rails initializers after Puma has already booted...

Is there still a case where this functionality is useful in Puma itself?

[alexeevit]

Since Puma itself doesn't need this functionality, and we can't do anything about people defining signal traps in their Rails initializers after Puma has already booted...
Is there still a case where this functionality is useful in Puma itself?

This functionality is not for Puma but mostly for app developers. Apparently, Puma get booted after Rails initializers and hence Puma overrides signal traps defined in initializers.

Therefore, I thought maybe Puma shouldn't override them. Initially I peeked the sidekiq's solution but then I found that there are some cases in the code when we define a trap and then override it in another place. In those cases the sidekiq's solution (call the overrided trap in the overriding one) is not suitable because it makes it call all the overrided traps. E.g. in Rails initializers we have trap1, in Puma we have trap2 and trap3. In Puma's code trap3 is supposed to completely override trap2. But we want to save trap1. So using the sidekiq's solution we're expecting trap3 to be trap3+trap1. But the sidekiq's solution will make trap3 to run trap3+trap2+trap1.

So I decided to let developers to inject their traps into Puma's ones. Maybe not the best solution but I'm certaintly open for discussion.

[alexeevit]

Maybe app developers should just be using something like this themselves.

For the case, when app's traps override Puma's ones - defenitely. But here I tried to solve the opposite problem - when Puma overrides app's traps.

[nateberkopec]

Looks like we have some test failures (the ones in TestWorkerGemIndependence are meaningful I believe)

[alexeevit]

Looks like we have some test failures (the ones in TestWorkerGemIndependence are meaningful I believe)

@nateberkopec I know that some tests fail. I decided to first get your approval whether the idea has a reason to live. So if you think that we should implement what I've done, I'll fix and polish everything.

[nateberkopec]

@alexeevit 😓 Sorry I was unclear! Yes, I believe this makes sense and we should merge (when ready).

[alexeevit]

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?

[MSP-Greg]

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

Could you rebase?

[alexeevit]

Done 👌

[MSP-Greg]

Thanks. Same problem as before... Sorry.

[alexeevit]

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

[alexeevit]

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.

[alexeevit]

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.

[dentarg]

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

[stanhu]

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

[alexeevit]

@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.

[jjb]

If the goal is to coexist with handlers added before or after puma adds one or more and for puma to not step on its own toes, I don't think the code to maintain the array of handlers is necessary. Ruby already maintains this array, one just needs to make sure to not overwrite it.

This can be done by using the approach mentioned by @mperham above:

https://github.com/mperham/sidekiq/blob/4ce86cda9e968de0d3aa198839c2dd0ac4c4f17b/lib/sidekiq/cli.rb#L48-L62

from my PR, see discussion:

sidekiq/sidekiq#4991

Using the technique from this fantastic book, which was recently put online 🤩 https://workingwithruby.com/wwup/signals/#signal-handlers-are-global

Maybe puma has special needs i'm not aware of (i don't know what "string handlers" are) but if not, I think this can be simplified so there is simply a reused method using a standard approach for each of these invocations.

[alexeevit]

If the goal is to coexist with handlers added before or after puma adds one or more and for puma to not step on its own toes, I don't think the code to maintain the array of handlers is necessary. Ruby already maintains this array, one just needs to make sure to not overwrite it.

This can be done by using the approach mentioned by @mperham above
<...>

Hey! Yes, the goal is to coexist with handlers added before puma adds its own. However, as I wrote before, the solution mentioned by you and Mike doesn't fully meets our requirements:

Initially I peeked the sidekiq's solution but then I found that there are some cases in the code when we define a trap and then override it in another place. In those cases the sidekiq's solution (call the overrided trap in the overriding one) is not suitable because it makes it call all the overrided traps. E.g. in Rails initializers we have trap1, in Puma we have trap2 and trap3. In Puma's code trap3 is supposed to completely override trap2. But we want to save trap1. So using the sidekiq's solution we're expecting trap3 to be trap3+trap1. But the sidekiq's solution will make trap3 to run trap3+trap2+trap1.

We don't need the array, but we should save the user's traps and call them in Puma traps.

i don't know what "string handlers" are

I don't really know if there's an official name for it. I meant the case, when you pass a string as the second argument to the Signal#trap instead of a closure, e.g. Signal.trap('SIGINT', 'SYSTEM_DEFAULT').

[nateberkopec]

Closing as "stuck". re: https://github.com/puma/puma/pull/2767/files#r872970473.

If you (or anyone else) figures this one out, please reopen.

[jjb]

@alexeevit thanks for the explanations - sorry for late response

there are some cases in the code when we define a trap and then override it in another place

I'm guessing this happens at boot time - maybe an array of lambdas or procs could be maintained during boot time, and then when done booting, register the handlers with ruby?

If it's a case of a condition making puma say "actually the handler should be this" then should be easy to replace one with another in the array.

If it's a case of a a subprocess and master process having different handlers, then maybe could still make it work - my "when done booting, register the handlers with ruby" step could be done independently for master and sub processes.

Regarding https://github.com/puma/puma/pull/2767/files#r872970473

looking at git diff v2_5_0 v2_6_0_preview1 (for some reason there is no 2_6_0 tag in my local even though there is on github, not sure why ruby/ruby@v2_5_0...v2_6_0)

• there is a new document, doc/signals.rdoc - nothing particularly illuminating there, but interesting that was created in the version where your problem started
  • here it is at 2.6 https://github.com/ruby/ruby/blob/v2_6_0/doc/signals.rdoc
  • and currently https://github.com/ruby/ruby/blob/master/doc/signals.rdoc
• other interesting things can be found by searching the diff for case-insensitive chld. there seems to be some behavior change, and there are some notes about behavior in MJIT, I don't fully grok it but probably relevant.