Add at_exit block automatically #397
Conversation
lib/bugsnag.rb
Outdated
| @@ -114,6 +120,21 @@ def notify(exception, auto_notify=false, &block) | |||
| end | |||
| end | |||
|
|
|||
| # Registers an at_exit function to automatically catch errors on exit | |||
| # This can disable by setting the 'add_exit_handler' option to False | |||
There was a problem hiding this comment.
This can be disabled by setting the 'add_exit_handler' option to false
|
I put it on a separate config option as at_exit blocks can interfere with exit codes. I didn't think it was a good idea to have at_exit blocks being registered automatically if users just want the rails/other framework exception hooks working. I don't believe it captures the kill signal, but I'm not sure it won't spit out the last exception in the app, even if it's been captured and dealt with already. I'll test this tomorrow. |
|
I'm pretty sure it will capture kill |
|
It doesnt look like the code here will alter the exit code? |
|
From my testing it doesn't capture kill signals in the example rails apps. Nor does it re-send the previous error if it's already been handled. Rather than linking it to auto_notify I'd rather have it opt-out, but still separate. |
|
Why? Python links it to auto_notify. Why the difference? |
|
The main difference is that this change could potentially be considered "breaking" for users who're already using custom at_exit blocks which we've advised users to do in the documentation to this point. |
|
How does it sound to put it behind a separate flag for now but when we do a major version bump we make a change to make it consistent with python? I like consistency across the notifiers because it makes support and integration easier for users. |
|
Provided @duncanhewett is happy with the documentation I'd like to get this merged and released. |
|
I would prefer to bump to v7 and make the at_exit block gated by auto_notify and on by default. |
|
I'd be happy with that, I'll have a look at what other changes could be made at the same time. |
|
Tests appear to be failing on this due to travis issues with Jruby. It installs and runs through tests fine locally. |
lib/bugsnag.rb
Outdated
| @@ -126,6 +124,40 @@ def notify(exception, auto_notify=false, &block) | |||
| end | |||
| end | |||
|
|
|||
| ## | |||
| # Registers an at_exit function to automatically catch errors on exit | |||
| # This can be disabled by setting the 'add_exit_handler' configuration option to false | |||
There was a problem hiding this comment.
The comment is outdated now
|
Can I get a re-review on this @snmaynard? |
|
I pushed a cleanup commit to this branch addressing my review comments. |
|
Thanks for that. For CI: Rubocop is complaining about module length - I would suggest again bumping up the module limit size and bringing forward a planned refactor. |
- Up Rubocop max module length
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.
For more information on puma's behaviour, see here:
puma/puma#1438
With the `at_exit` handler introduced in bugsnag#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`.
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.
With #397 we receive SignalException error reports when puma shuts down, which aren't really errors.
* Add SignalException to default ignore_classes With #397 we receive SignalException error reports when puma shuts down, which aren't really errors. * Remove exit exception checks in integrations These are now in the default ignore_classes. * Improve IgnoredExceptions: Remove exception rejection from sidekiq integration
Adds a configuration options and method to register a pre-defined at_exit block when configuration is run.
register_at_exitfunction accessible on library to add at_exit block with severity reasoning includedadd_exit_handlerconfiguration option automatically callsregister_at_exitif set to true, and is flagged to only do so onceadd_exit_handlerdefaults to false to avoid issues in frameworks (for instance can blockexit 1signals in rails)