Skip to content

Commit

Permalink
Do not report SignalExceptions in at_exit handler
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mrnugget committed Jul 20, 2018
1 parent f832296 commit 817dc77
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 30 deletions.
20 changes: 11 additions & 9 deletions lib/bugsnag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -118,15 +118,17 @@ def notify(exception, auto_notify=false, &block)
def register_at_exit
return if at_exit_handler_installed?
@exit_handler_added = true
at_exit do
if $!
Bugsnag.notify($!, true) do |report|
report.severity = 'error'
report.severity_reason = {
:type => Bugsnag::Report::UNHANDLED_EXCEPTION
}
end
end
at_exit { $! && at_exit_handler($!) }
end

def at_exit_handler(exception)
return if exception.is_a?(SignalException)

Bugsnag.notify(exception, true) do |report|
report.severity = 'error'
report.severity_reason = {
:type => Bugsnag::Report::UNHANDLED_EXCEPTION
}
end
end

Expand Down
67 changes: 46 additions & 21 deletions spec/bugsnag_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,33 +41,58 @@
Bugsnag.register_at_exit
end

describe 'at_exit_handler' do
context 'with a non-signal exception' do
it 'reports the exception' do
exception = BugsnagTestException.new('Oh no')

report_mock = double('report')
expect(report_mock).to receive(:severity=).with('error')
expect(report_mock).to receive(:severity_reason=).with({
:type => Bugsnag::Report::UNHANDLED_EXCEPTION
})

expect(Bugsnag).to receive(:notify).with(exception, true).and_yield(report_mock)

Bugsnag.at_exit_handler(exception)
end
end

context 'with a signal exception' do
it 'does not report the exception' do
exception = SignalException.new('SIGTERM')

expect(Bugsnag).to_not receive(:notify).with(exception, true)

Bugsnag.at_exit_handler(exception)
end
end
end

context 'with aliased at_exit' do
before do
module Kernel
alias_method :old_at_exit, :at_exit
def at_exit
begin
raise BugsnagTestException.new("Oh no")
rescue
yield
context 'and a non-signal exception' do
before do
module Kernel
alias_method :old_at_exit, :at_exit
def at_exit
begin
raise BugsnagTestException.new("Oh no")
rescue
yield
end
end
end
end
end

it "sends an exception when at_exit is called" do
report_mock = double('report')
expect(report_mock).to receive(:severity=).with('error')
expect(report_mock).to receive(:severity_reason=).with({
:type => Bugsnag::Report::UNHANDLED_EXCEPTION
})
expect(Bugsnag).to receive(:notify).with(kind_of(BugsnagTestException), true).and_yield(report_mock)
Bugsnag.register_at_exit
end
it "calls the at_exit_handler" do
expect(Bugsnag).to receive(:at_exit_handler)
Bugsnag.register_at_exit
end

after do
module Kernel
alias_method :at_exit, :old_at_exit
after do
module Kernel
alias_method :at_exit, :old_at_exit
end
end
end
end
Expand Down

0 comments on commit 817dc77

Please sign in to comment.