-
Notifications
You must be signed in to change notification settings - Fork 173
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
before_notify_callbacks don't work with Sidekiq #417
Comments
Hi @tsujigiri, the reason is because we store the You can see this from adding checks before the callback is registered and in the worker. |
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? |
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. |
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? |
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. |
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? |
Here is a link to someone doing something similar yesterday -> #449 (comment) |
So if we want consistent callback handling across Rails and Sidekiq we should stop using |
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. |
That does help. Thank you. The information we need to add to the report is constant so the solution should work for us. |
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
BUGSNAG_API_KEY
environment variable needs to be set.bundle exec sidekiq
Bugsnag.notify(StandardError.new('foo'))
is being run from the console directly, we end up in the callback.FooWorker
is run viaFooWorker.perform_async
from the Rails console, the callback in the initializer is not called.Version
See Gemfile.lock
The text was updated successfully, but these errors were encountered: