Extract "execute" method to reduce "perform" method complexity #138
Conversation
|
|
||
| retry_or_discard_error = GoodJob::CurrentExecution.error_on_retry || | ||
| GoodJob::CurrentExecution.error_on_discard | ||
|
|
||
| error = nil | ||
| if rescued_error | ||
| error = rescued_error | ||
| elsif result.is_a?(Exception) |
There was a problem hiding this comment.
Do you know why/when the result can be an exception? I find it quite surprising.
There was a problem hiding this comment.
Good question! Apologies if this turns into story time.
Briefly: when using ActiveJob.rescue_from (or retry_on / discard_on which are implemented with rescue_from), ActiveJob will return the exception that is caught/handled.
Storytime...
I'm not sure whether the above behavior is intentional or merely a coincidence.
The behavior is unreliable though. I discovered in production that Sentry's instrumentation interrupts that behavior: https://github.com/getsentry/raven-ruby/blob/a5f101a9c7776168df1c0e2aac13d46b70bbb24e/lib/raven/integrations/rails/active_job.rb#L21-L31
...you'll notice that Sentry will call rescue_with_handler but does not return the result. I just checked it off my todo list by submitting a PR (thanks for the inspiration 🎉 ): getsentry/sentry-ruby#1027 ...but I have to assume that other gems may also interrupt this behavior too. This is explored in #72.
There was a problem hiding this comment.
I think this is a great refactor.
A weakly held stylistic thought is that result, rescued_error = execute does visually understate THIS IS WHERE THE WORK HAPPENS and I could see it being overlooked by someone visiting this file with fresh eyes.
...but that's not really actionable feedback. I will merge this and keep it in mind 🙌
While looking at a way to implement #136 I found perform method quite complex and was trying to simplify it a bit.
Feel free to close it if you prefer to have it in one method.