-
-
Notifications
You must be signed in to change notification settings - Fork 494
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
Gracefully shutdown background worker before the process exits #1617
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1617 +/- ##
==========================================
- Coverage 98.51% 96.13% -2.38%
==========================================
Files 133 107 -26
Lines 7394 6105 -1289
==========================================
- Hits 7284 5869 -1415
- Misses 110 236 +126
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very cool! I was just explaining this to support the other day why his test code wasn't working because the parent process died without flushing.
Thanks for adding!
@sl0thentr0py np 👍 |
@@ -19,9 +19,7 @@ module Task | |||
def execute(args=nil) | |||
return super unless Sentry.initialized? && Sentry.get_current_hub | |||
|
|||
Sentry.get_current_hub.with_background_worker_disabled do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at execute
here — isn't it just not doing anything in this form? Looks like it falls back to super
in every branch, no? /cc @st0012
Currently, when a process exits, SDK's background worker will discard all WIP sending tasks and cause lost of events. This is why we need to disable background worker in rake integration. And from the discussion in #1612, it seems to cause issues in the resque integration too.
Even though I want to avoid using the
at_exit
block, I don't think the responsibility of shutting down the background worker should fall on users. It should be handled by the SDK whenever possible. So in addition to providing a shutdown API in this PR, the SDK would also start the shutdown procedure when exiting the process.Closes #1612