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

sentry + eager_load breaks ActiveJob rescue_from #1249

Closed
CroneKorkN opened this issue Jan 29, 2021 · 3 comments · Fixed by #1464 or #1494
Closed

sentry + eager_load breaks ActiveJob rescue_from #1249

CroneKorkN opened this issue Jan 29, 2021 · 3 comments · Fixed by #1464 or #1494
Assignees
Projects
Milestone

Comments

@CroneKorkN
Copy link

Describe the bug

Sentry eats errors in Jobs. They return nil instead.

To Reproduce

Expected behavior

This line should never be executed because the line before it throws an error: https://github.com/CroneKorkN/sentry_broken_with_eager_load/blob/master/app/jobs/error_job.rb#L4

You should not see >>> I SHOULD NEVER BE EXECUTED! (NilClass)

Actual behavior

The line is executed because block.call returns nil.

You see >>> I SHOULD NEVER BE EXECUTED! (NilClass)

Environment

  • Ruby Version: 2.7.1
  • Sentry-Rails: 4.1.6
  • Rails: 6.1.0

Raven Config

https://github.com/CroneKorkN/sentry_broken_with_eager_load/blob/master/config/initializers/sentry.rb

Test Repo

On this commit, the error occurs: https://github.com/CroneKorkN/sentry_broken_with_eager_load/tree/0734f3648f9687c27f3f8ea505b1b26f5975cd97
This commit fixes it by disabling eager_load: https://github.com/CroneKorkN/sentry_broken_with_eager_load/tree/ddf2e41ebee1b31afb5c533601048ae740bb00f6

Removing the sentry-rails gem obviously also fixes it.

@CroneKorkN
Copy link
Author

This return should not happen, from my limited understanding: https://github.com/getsentry/sentry-ruby/blob/master/sentry-rails/lib/sentry/rails/active_job.rb#L30

@st0012 st0012 added this to To do in 4.x via automation Jan 29, 2021
@st0012
Copy link
Collaborator

st0012 commented May 28, 2021

@CroneKorkN we've introduced several improvements to the SDK since 4.1.6, can you try the newest 4.5.0 and see if the issue still exists?

@st0012
Copy link
Collaborator

st0012 commented Jun 3, 2021

Update: I can now reproduce the issue and have found the cause.

Context

Rails' initializers runs in this order:

  1. Built-in initializers (like the eager_load! initializer)
  2. Config initializers (files under config/initializers)
  3. The after_initialize callback (where sentry-rails injects all the patches)

Because we want to avoid injecting any Sentry extension into Rails unless the SDK is initialized (some users don't even initialize the SDK in dev environment), all patches are applied in the after_initialize phase.

The Problem

This approach works fine for most patches, but it's the root cause of this issue. The reason is that the ActiveJob extension works by injecting around_perform callback, which is order sensitive.

When config.eager_load = false

The callback registering order looks like this:

  1. The eager_load! initializer does not run. No user jobs loaded.
  2. The after_initialize is called and the SDK registers its around_perform callback.
  3. When ErrorJob.perform_later is called, the job class is lazily loaded and then its around_perform callback is loaded.

So ErrorJob's callback chain looks like this:

  • other callbacks
    • SDK's around_perform
      • ErrorJob's around_perform

When config.eager_load = true

Now the order gets different:

  1. The eager_load! initializer runs and loads ErrorJob.
  2. ErrorJob registers its around_perform callback.
  3. The after_initialize is called and the SDK registers its around_perform callback.

And the ErrorJob's callback chain becomes:

  • other callbacks
    • ErrorJob's around_perform
      • SDK's around_perform

Solution

To make sure ActiveJob capturing works as expected, we need to make sure it's never registered after the user's callback. So sentry-rails needs to apply the ActiveJob patch before the eager_load! initializer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment