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

Calling the error handler twice when raising an exception in a rescue_from block #956

Closed
donaldong opened this issue Apr 10, 2020 · 3 comments · Fixed by #1631
Closed

Calling the error handler twice when raising an exception in a rescue_from block #956

donaldong opened this issue Apr 10, 2020 · 3 comments · Fixed by #1631

Comments

@donaldong
Copy link

donaldong commented Apr 10, 2020

When using an ActiveJob Adapter which is not listed in

https://github.com/getsentry/raven-ruby/blob/ff59e93057ac8ac98603f8745f5024df7ed47dc2/lib/raven/integrations/rails/active_job.rb#L4

capture_and_reraise_with_sentry calls rescue_with_handler
https://github.com/getsentry/raven-ruby/blob/ff59e93057ac8ac98603f8745f5024df7ed47dc2/lib/raven/integrations/rails/active_job.rb#L21-L24

However, if the error handler (defined by a rescue_from) raises an error, the same error will be reported to the error handler itself again (double calling the error handler).

Curious what would be a proper workaround? If no workaround seems reasonable, can we provide a way to skip capture_and_reraise_with_sentry (or simply disable the ActiveJobExtensions)?

@donaldong donaldong changed the title Raising an exception in a rescue_from block Calling the error handler twice when raising an exception in a rescue_from block Apr 10, 2020
@st0012 st0012 added this to Needs triage in 3.x Aug 6, 2020
@st0012 st0012 added this to the 3.1.0 milestone Aug 14, 2020
@st0012
Copy link
Collaborator

st0012 commented Aug 28, 2020

thanks for reporting, can you provide a minimum app that can reproduce the issue? thanks

@st0012 st0012 removed this from the 3.1.0 milestone Sep 10, 2020
@st0012 st0012 removed this from Needs triage in 3.x Sep 10, 2020
@st0012 st0012 closed this as completed Dec 10, 2020
@sinshutu
Copy link

@st0012

I was able to reproduce it in my environment.
I've also created a minimum app.

see https://github.com/sinshutu/sample-rails-sentry-calling-the-error-handler-twice/tree/main

Just run ./setup.sh and you will see it run twice!

@st0012 st0012 reopened this Nov 11, 2021
@st0012
Copy link
Collaborator

st0012 commented Nov 11, 2021

@sinshutu thanks for the feedback. I've reopened this issue and will check it later.

@st0012 st0012 self-assigned this Nov 27, 2021
@st0012 st0012 added this to To do in 4.x via automation Nov 27, 2021
@st0012 st0012 added this to the 4.9.0 milestone Nov 27, 2021
st0012 added a commit that referenced this issue Nov 27, 2021
since the sdk registers its around_perform as the "first" callback, it
needs to call all error handlers manually (which naturally should happen
later) to see if it should report the exception.

usually, this approach works well because if an exception will be
handled later, handling it ealier doesn't make much of a difference.

however, as described in #956, if there's another exception happened
inside one of the error handlers, it'll re-enter the error handler again
in here: https://github.com/rails/rails/blob/38cb5610b1/activejob/lib/active_job/execution.rb#l50-l51

Of course, it may be possible to handle such case by adding more rescue
block inside the `capture_and_reraise_with_sentry` method. But it'll
force the entire exception handling flow to be performed inside the
first `around_perform` block. And that's something we should avoid.
4.x automation moved this from To do to Done Dec 1, 2021
st0012 added a commit that referenced this issue Dec 1, 2021
…ion (#1631)

* use prepended method instead of around_perform for activejob integration

since the sdk registers its around_perform as the "first" callback, it
needs to call all error handlers manually (which naturally should happen
later) to see if it should report the exception.

usually, this approach works well because if an exception will be
handled later, handling it ealier doesn't make much of a difference.

however, as described in #956, if there's another exception happened
inside one of the error handlers, it'll re-enter the error handler again
in here: https://github.com/rails/rails/blob/38cb5610b1/activejob/lib/active_job/execution.rb#l50-l51

Of course, it may be possible to handle such case by adding more rescue
block inside the `capture_and_reraise_with_sentry` method. But it'll
force the entire exception handling flow to be performed inside the
first `around_perform` block. And that's something we should avoid.

* Refactor ActiveJob integration

* Fix SendEventJob's rescue_from callback

* Update changelog
@st0012 st0012 modified the milestones: 4.9.0, 4.8.2 Dec 4, 2021
@st0012 st0012 modified the milestones: 4.8.2, 4.9.0 Dec 25, 2021
st0012 added a commit that referenced this issue Jan 4, 2022
…ion (#1631)

* use prepended method instead of around_perform for activejob integration

since the sdk registers its around_perform as the "first" callback, it
needs to call all error handlers manually (which naturally should happen
later) to see if it should report the exception.

usually, this approach works well because if an exception will be
handled later, handling it ealier doesn't make much of a difference.

however, as described in #956, if there's another exception happened
inside one of the error handlers, it'll re-enter the error handler again
in here: https://github.com/rails/rails/blob/38cb5610b1/activejob/lib/active_job/execution.rb#l50-l51

Of course, it may be possible to handle such case by adding more rescue
block inside the `capture_and_reraise_with_sentry` method. But it'll
force the entire exception handling flow to be performed inside the
first `around_perform` block. And that's something we should avoid.

* Refactor ActiveJob integration

* Fix SendEventJob's rescue_from callback

* Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.x
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants