Do not report SignalExceptions in at_exit handler

[mrnugget]

Goal

The previous behaviour resulted in applications reporting errors even though they were gracefully and without any problems shut down. Example: the puma webserver relies on SIGTERM to gracefully shut down its workers and Bugsnag reported errors when it was normally shut down.

With the at_exit handler introduced in #397, Bugsnag would catch SignalExceptions on exit and report them as error, even though they are none.

This PR changes the existing behaviour to add SignalException as a special case in which the exception should not be passed to Bugsnag.notify.

Changeset

Added

• Bugsnag.at_exit_handler

Changed

• Bugsnag.register_at_exit

Tests

Due to the Ruby exiting when a SignalException is raised I had to refactor the code to make the actual handling of the exit-exceptions testable without the test suite quitting.

Linked issues

For more information on puma's behaviour, see here: puma/puma#1438
Honeybadger also ignores SignalExceptions: honeybadger-io/honeybadger-ruby#267

Review

For the submitter, initial self-review:

• Commented on code changes inline explain the reasoning behind the approach
• Reviewed the test cases added for completeness and possible points for discussion
• A changelog entry was added for the goal of this pull request
• Check the scope of the changeset - is everything in the diff required for the pull request?
• This pull request is ready for:
  • Initial review of the intended approach, not yet feature complete
  • Structural review of the classes, functions, and properties modified
  • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

• Consistency across platforms for structures or concepts added or modified
• Consistency between the changeset and the goal stated above
• Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
• Usage friction - is the proposed change in usage cumbersome or complicated?
• Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
• Concurrency concerns - if components are accessed asynchronously, what issues will arise
• Thoroughness of added tests and any missing edge cases
• Idiomatic use of the language

[bengourley]

Hi @mrnugget, thanks for the PR. At a glance your description of the problem sounds reasonable and the current behaviour undesirable. I'll schedule a proper look at this PR and see where we should take it from here. Cheers!

[soulcutter]

[snmaynard]

How would we feel about signalexceptions being added to the default ignoreclasses? I'd rather have this behaviour be configurable by users with a sensible default. Right now you can achieve this by adding SignalException to the ignore classes.

Having said that we already have something like this in other integrations like

bugsnag-ruby/lib/bugsnag/integrations/sidekiq.rb

Line 25 in 35b3ef0

raise ex if [Interrupt, SystemExit, SignalException].include? ex.class

.

I think I'd favour removing all the special cases and ignoring signal exceptions. Thoughts?

[mrnugget]

How would we feel about signalexceptions being added to the default ignoreclasses?

That's probably the cleanest of all solutions! I'd prefer it, yes.

Also didn't know that there were default ignoreclasses 🙈. A quick search got me to this PR which seems to do the same for other two exceptions in the Sidekiq integration:

#404

So, yes, I strongly agree, let's add SignalException to this set 👍

[bengourley]

I'm going to close this PR as you can add SignalException to the list of ignore_classes in your app's configuration to achieve the desired behaviour.

I'm also going to make an internal backlog ticket to add this to the default list of ignore_classes, and to remove the specific logic in the sidekiq integration (and any others where it exists).

Thanks all!