Skip to content

Gracefully shutdown background worker before the process exits#1617

Merged
st0012 merged 3 commits intomasterfrom
fix-#1612
Nov 22, 2021
Merged

Gracefully shutdown background worker before the process exits#1617
st0012 merged 3 commits intomasterfrom
fix-#1612

Conversation

@st0012
Copy link
Copy Markdown
Contributor

@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
Copy Markdown

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
@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
@st0012 st0012 requested a review from sl0thentr0py November 22, 2021 17:57
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

st0012 commented Nov 22, 2021

@sl0thentr0py np 👍

@st0012 st0012 merged commit 46e7072 into master Nov 22, 2021
@st0012 st0012 deleted the fix-#1612 branch November 22, 2021 18:09
def execute(args=nil)
return super unless Sentry.initialized? && Sentry.get_current_hub

Sentry.get_current_hub.with_background_worker_disabled do
Copy link
Copy Markdown
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

None yet

Development

Successfully merging this pull request may close these issues.

Synchronously drain async error reporting queue

4 participants