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

coexist with other signal handlers #4991

Merged
merged 3 commits into from Sep 13, 2021

Conversation

jjb
Copy link
Contributor

@jjb jjb commented Sep 11, 2021

Using a technique I learned from the fantastic Working With Unix Processes, this allows other signal handlers to coexist with sidekiq. Simple example:

config/initializers/sidekiq.rb

Sidekiq.configure_server do |config|
  trap('INT') do
    puts "TRAPPED!"
  end
end

Before this PR, this will never run. With the PR, this will run before the sidekiq behavior runs.

I tried writing a test in test_cli.rb and spent longer than I'd care to admit before I realized the tests there don't send signals, they just invoke the method.

Are there any other tests in the suite which do run processes and send signals? If so I'll try to put something in there.

Let me know what you think!

@mperham
Copy link
Collaborator

mperham commented Sep 12, 2021

I've found that this code changes so little and is so hard to test that I typically manually test it.

Should we be concerned at all with the old handler raising errors?

@jjb
Copy link
Contributor Author

jjb commented Sep 12, 2021

Should we be concerned at all with the old handler raising errors?

Good thinking, I pushed up a commit to accommodate this. This is probably definitely the behavior wanted for signals like INT and TERM. For other signals, there are theoretically cases where errors should maybe cause the server to shut down instead of for sidekiq to go forward witih its business. But even in these cases, the person catching the signals will know what they are doing, and if they think sidekiq should shut down in the case of error they should code that themselves.

For less experienced people, the base behavior that sidekiq will log the error and carry forward is I think what they will be happy with, and the best behavior for avoiding support load for you.

n.b. most of the time if someone knows what they are doing they will use at_exit, which is not affected by the current behavior or the new behavior in this PR. If someone needs some code to run before exit, it's usually doing something like cleaning up work in a queue.

If an error is raised in at_exit, sidekiq shutdown will raise an exception. Good news is, sidekiq will do 100% of its shutdown before ruby runs the at_exit code.

config/initializers/sidekiq.rb

Sidekiq.configure_server do |config|
  at_exit do
    raise "error raised in at_exit"
  end
end
^C2021-09-12T22:58:40.310Z pid=60044 tid=5cs INFO: Shutting down
2021-09-12T22:58:40.310Z pid=60044 tid=5cs INFO: Terminating quiet workers
2021-09-12T22:58:40.312Z pid=60044 tid=4fc INFO: Scheduler exiting...
2021-09-12T22:58:40.418Z pid=60044 tid=5cs INFO: Pausing to allow workers to finish...
2021-09-12T22:58:40.839Z pid=60044 tid=5cs INFO: Bye!
Traceback (most recent call last):
<snip>
/Users/john/src/sidekiq/lib/sidekiq/cli.rb:166:in `block in <class:CLI>': Interrupt (Interrupt)
<snip>
/Users/john/src/sidekiq/lib/sidekiq/cli.rb:127:in `exit': exit (SystemExit)
<snip>
/Users/john/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/bundler-2.2.19/lib/bundler/friendly_errors.rb:137:in `exit': exit (SystemExit)
/Users/john/healthie/web/config/initializers/sidekiq.rb:13:in `block (2 levels) in <top (required)>': error raised in at_exit (RuntimeError)

@jjb
Copy link
Contributor Author

jjb commented Sep 12, 2021

thoughts on semver:

If someone is using a library that has signal handling code and they aren't aware of it, and that code now becomes problematic, then this change will start to cause problems for them. So they might do a tiny or minor sidekiq version update and find their code having problems. I think it's very uncommon, probably approaching the never category, for libraries which aren't themselves servers to have signal handling code, so this can probably be ignored.

Slightly more common, but also very uncommon, is people who have signal handling code in their own app, which they want run in the context of their app running on the web, and now that code will suddenly start running in sidekiq as well. I'm thinking this is also uncommon enough to be ignored, and hey, people should be reading the changelog before updating anyway.

@mperham
Copy link
Collaborator

mperham commented Sep 12, 2021

Signal handlers can’t use a Logger but I’m not sure if that includes puts. Do you know?

@jjb
Copy link
Contributor Author

jjb commented Sep 12, 2021

I believe that's only because the rails logger (and maybe also stdlib Logger, not sure) use a mutex. some googling seems to confirm this (and brought me to your ticket from 8 years ago :-D https://bugs.ruby-lang.org/issues/7917 )

I just did some quick tests with a custom signal handler, puts succeeds, Rails.logger.info gives "log writing failed. can't be called from trap context".

@mperham mperham merged commit 582b57a into sidekiq:master Sep 13, 2021
mperham added a commit that referenced this pull request Sep 13, 2021
@jjb jjb deleted the coexist-with-other-signal-handlers branch September 13, 2021 16:53
@jjb
Copy link
Contributor Author

jjb commented Sep 13, 2021

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants