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

Gracefully shutdown background worker before the process exits #1617

Merged
merged 3 commits into from
Nov 22, 2021

Conversation

st0012
Copy link
Collaborator

@st0012 st0012 commented Nov 13, 2021

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

@st0012 st0012 self-assigned this Nov 13, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 13, 2021

Codecov Report

Merging #1617 (d544a63) into master (bf37772) will decrease coverage by 2.37%.
The diff coverage is 76.92%.

❗ Current head d544a63 differs from pull request most recent head 84715a1. Consider uploading reports for the commit 84715a1 to get more accurate results
Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/breadcrumb.rb 100.00% <ø> (ø)
sentry-ruby/lib/sentry/breadcrumb/sentry_logger.rb 100.00% <ø> (ø)
sentry-ruby/lib/sentry/breadcrumb_buffer.rb 100.00% <ø> (ø)
sentry-ruby/lib/sentry/client.rb 100.00% <ø> (ø)
sentry-ruby/lib/sentry/configuration.rb 98.41% <ø> (ø)
sentry-ruby/lib/sentry/core_ext/object/deep_dup.rb 94.11% <ø> (ø)
...ntry-ruby/lib/sentry/core_ext/object/duplicable.rb 60.00% <ø> (ø)
sentry-ruby/lib/sentry/dsn.rb 100.00% <ø> (ø)
sentry-ruby/lib/sentry/envelope.rb 100.00% <ø> (ø)
sentry-ruby/lib/sentry/exceptions.rb 100.00% <ø> (ø)
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf37772...84715a1. Read the comment docs.

@st0012 st0012 added this to the 5.0.0 milestone Nov 13, 2021
@st0012 st0012 added this to In progress in 4.x via automation Nov 13, 2021
@rhcarvalho rhcarvalho changed the title Gracefully shutdown background worker before the process exists Gracefully shutdown background worker before the process exits Nov 16, 2021
@st0012 st0012 modified the milestones: 5.0.0, 4.8.1 Nov 19, 2021
Copy link
Member

@sl0thentr0py sl0thentr0py left a 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!

@st0012
Copy link
Collaborator Author

st0012 commented Nov 22, 2021

@sl0thentr0py np 👍

@st0012 st0012 merged commit 46e7072 into master Nov 22, 2021
4.x automation moved this from In progress to Done Nov 22, 2021
@st0012 st0012 deleted the fix-#1612 branch November 22, 2021 18:09
@@ -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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.x
  
Done
Development

Successfully merging this pull request may close these issues.

Synchronously drain async error reporting queue
4 participants