Skip to content

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+25 −4
Draft

fix(sessions): clean up threading on kill+flush #4562

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c495735
fix(sessions): clean up threading on kill+flush
bukzor Jul 8, 2025
8d24e88
fix after rebase
bukzor Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions sentry_sdk/sessions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,9 @@ def __init__(

def flush(self):
# type: (...) -> None
pending_sessions = self.pending_sessions
self.pending_sessions = []
with self._thread_lock:
pending_sessions = self.pending_sessions
self.pending_sessions = []

with self._aggregate_lock:
pending_aggregates = self.pending_aggregates
Expand All @@ -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():
Copy link
Member

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?

return
with self._thread_lock:
if not self._should_join_thread():
return
if self._thread: # typing
self._thread.join()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@untitaker untitaker Jul 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

self._thread = None
self._thread_for_pid = None

def _should_join_thread(self):
# type: (...) -> bool
return (
not self.__shutdown_requested.is_set()
and self._thread is not None
# we are the parent thread:
and self._thread_for_pid == os.getpid()
)

def _ensure_running(self):
# type: (...) -> None
"""
Check that we have an active thread to run in, or create one if not.

Note that this might fail (e.g. in Python 3.12 it's not possible to
spawn new threads at interpreter shutdown). In that case self._running
will be False after running this function.
spawn new threads at interpreter shutdown). In that case
`__shutdown_requested` will be set after running this function.
"""
if self._thread_for_pid == os.getpid() and self._thread is not None:
return None
Expand Down
Loading