fix(sessions): clean up threading on kill+flush #4562
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.
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.
bukzor
force-pushed
the
bukzor/can-wait4-session-flusher
branch
from
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.
why are two of these required? isn't the one under the lock sufficient?
| if not self._should_join_thread(): | ||
| return | ||
| if self._thread: # typing | ||
| self._thread.join() |
There was a problem hiding this comment.
sorry but I actually don't understand this change at all properly
the flush function is called by the thread in the loop, so joining that thread from within flush makes no sense, it should be done from the calling thread?
There was a problem hiding this comment.
yeah this might deadlock if flush is called from within the thread. the goal was to make it so that client.close() blocks until the thread is actually gone. client also calls flush directly, and in that call, blocking here makes sense.
proposal: let's block in kill() until the thread is gone, or make flush take a kwarg like flush(join_thread=True)
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.