before_notify_callbacks don't work with Sidekiq

[tsujigiri]

Expected behavior

When an exception is raised in a Sidekiq worker, it should also run the before_notify_callbacks.

Observed behavior

It doesn't. 🤷

Steps to reproduce

I set up a new Rails app and added sidekiq and bugsnag: https://github.com/ad2games/sidekiq_bugsnag_test

• Before running, the BUGSNAG_API_KEY environment variable needs to be set.
• Sidekiq needs to be started: bundle exec sidekiq
• When Bugsnag.notify(StandardError.new('foo')) is being run from the console directly, we end up in the callback.
• When FooWorker is run via FooWorker.perform_async from the Rails console, the callback in the initializer is not called.

Version

See Gemfile.lock

[Cawllec]

Hi @tsujigiri, the reason is because we store the request_data (including the callbacks) in Thread.current, meaning when Sidekiq spawns a thread to run FooWorker and the request_data is attached to the report it won't be the same array.

You can see this from adding checks before the callback is registered and in the worker.
The results I got were:
"#<Thread:0x007fbfe007ef70>:Registering callback"
and:
"#<Thread:0x007fbfe0ab5a70>:PERFORMING TASK".
I'm not entirely sure how you'd fix this aside from running before_notify_callbacks << in the worker itself. We'll have to look at how the client can deal with storing this data across threads.

[tsujigiri]

Why is it necessary to store the callbacks in the thread? They shouldn't ever change other than as part of the initial configuration, right?

[Cawllec]

They allow you to add callbacks scoped to a particular request, which won't be retained when the thread closes, stopping memory leaks and retention of things like user information in memory.

I'll make sure the need to initialise the callback in the task rather than the server is documented properly.

[tsujigiri]

This seems like a bit of an edge-case to me and it forces everyone to make sure every thread has the callbacks set, that are supposed to always be called. If you think thread-local callbacks are really necessary, maybe having both options, global callbacks as well as thread-local ones, would be a good compromise?

[Cawllec]

Hi @tsujigiri, sorry for the delay. We're currently considering any further action or API changes we could make to simplify how callbacks work. We'll keep this thread updated as necessary.

[zwass]

Disappointed to see that this is still unsolved and undocumented besides this issue. Can someone please provide an example of how to set the callback inside the thread?

[snmaynard]

Here is a link to someone doing something similar yesterday -> #449 (comment)

[zwass]

So if we want consistent callback handling across Rails and Sidekiq we should stop using Bugsnag.before_notify_callbacks, and instead add a middleware that decorates the errors in the same way?

[snmaynard]

It depends what you want. If during a controller action you want to say before any notify that happens from now until the end of the request, add this information to the report - then you will want to use the documented before_notify_callbacks method. If you just want one constant before notify, then you can use the middleware. The middleware will have no access to any request level context.

Does that help clarify? It really depends what you are trying to do.

[zwass]

That does help. Thank you. The information we need to add to the report is constant so the solution should work for us.