-
Notifications
You must be signed in to change notification settings - Fork 554
fix(sessions): clean up threading on kill+flush #4562
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #4562 +/- ##
==========================================
- Coverage 80.68% 80.65% -0.03%
==========================================
Files 156 156
Lines 16482 16494 +12
Branches 2802 2805 +3
==========================================
+ Hits 13299 13304 +5
- Misses 2299 2306 +7
Partials 884 884
|
This needs test coverage, apparently. Any hints? I'll do the work. |
sentry_sdk/sessions.py
Outdated
def _thread_stopping(self): | ||
# type: (...) -> bool | ||
return ( | ||
not self._running |
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.
there's no more self._running
so will wait till you rebase on the other change
This follows the precedent set in transport.flush, and helps me with my project to assert thread hygeine in sentry test suite (related: DI-1008), but also makes the system much more deterministic for everyone. This will cause shutdown to be quite slow until #4561 goes in. So I'd reject this if you reject that.
cf8a3ed
to
8d24e88
Compare
@@ -190,14 +191,34 @@ def flush(self): | |||
if len(envelope.items) > 0: | |||
self.capture_func(envelope) | |||
|
|||
# hygiene: deterministically clean up any stopping thread | |||
if not self._should_join_thread(): |
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.
why are two of these required? isn't the one under the lock sufficient?
This follows the precedent set in BackgroundWorker.flush, and helps me with my project
to assert thread hygeine in sentry test suite (related: DI-1008), but also makes
the system much more deterministic for everyone.
This will cause shutdown to be quite slow until #4561 goes in. So I'd reject
this if you reject that.