From 58568fdfbfefe56569d34a58f42b007b3b37a41d Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Mon, 30 Oct 2023 17:18:26 +0100 Subject: [PATCH 1/9] Handle failure during thread creation --- sentry_sdk/monitor.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/sentry_sdk/monitor.py b/sentry_sdk/monitor.py index 5a45010297..cf4aa20e86 100644 --- a/sentry_sdk/monitor.py +++ b/sentry_sdk/monitor.py @@ -53,7 +53,14 @@ def _thread(): thread = Thread(name=self.name, target=_thread) thread.daemon = True - thread.start() + try: + thread.start() + except RuntimeError: + # Unfortunately at this point the interpreter is in a state that no + # longer allows us to spawn a thread and we have to bail. + self._running = False + return None + self._thread = thread self._thread_for_pid = os.getpid() From a24a1c40fc46edab41784b09cbe244d2c2341eaf Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 31 Oct 2023 08:54:21 +0100 Subject: [PATCH 2/9] Handling starting threads during startup --- sentry_sdk/profiler.py | 8 +++++++- sentry_sdk/sessions.py | 10 +++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index 7ae73b056e..c433a54a73 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -918,7 +918,13 @@ def ensure_running(self): # can keep the application running after other threads # have exited self.thread = threading.Thread(name=self.name, target=self.run, daemon=True) - self.thread.start() + try: + self.thread.start() + except RuntimeError: + # Unfortunately at this point the interpreter is in a start that no + # longer allows us to spawn a thread and we have to bail. + self.running = False + return def run(self): # type: () -> None diff --git a/sentry_sdk/sessions.py b/sentry_sdk/sessions.py index 520fbbc059..f2aeb91265 100644 --- a/sentry_sdk/sessions.py +++ b/sentry_sdk/sessions.py @@ -120,9 +120,17 @@ def _thread(): thread = Thread(target=_thread) thread.daemon = True - thread.start() + try: + thread.start() + except RuntimeError: + # Unfortunately at this point the interpreter is in a state that no + # longer allows us to spawn a thread and we have to bail. + self._running = False + return None + self._thread = thread self._thread_for_pid = os.getpid() + return None def add_aggregate_session( From 60b5e32b1ab9c213b9c5e0bcd7a93481c7689a0c Mon Sep 17 00:00:00 2001 From: Anton Pirker Date: Tue, 31 Oct 2023 09:07:57 +0100 Subject: [PATCH 3/9] typos --- sentry_sdk/metrics.py | 2 +- sentry_sdk/profiler.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sentry_sdk/metrics.py b/sentry_sdk/metrics.py index bc91fb9fb7..fe8e86b345 100644 --- a/sentry_sdk/metrics.py +++ b/sentry_sdk/metrics.py @@ -348,7 +348,7 @@ def _ensure_thread(self): try: self._flusher.start() except RuntimeError: - # Unfortunately at this point the interpreter is in a start that no + # Unfortunately at this point the interpreter is in a state that no # longer allows us to spawn a thread and we have to bail. self._running = False return False diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index c433a54a73..400767fd51 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -921,7 +921,7 @@ def ensure_running(self): try: self.thread.start() except RuntimeError: - # Unfortunately at this point the interpreter is in a start that no + # Unfortunately at this point the interpreter is in a state that no # longer allows us to spawn a thread and we have to bail. self.running = False return From 25b87a6db3092f5edbaa8f90c3b119bc1ca14cc7 Mon Sep 17 00:00:00 2001 From: Ivana Kellyerova Date: Thu, 2 Nov 2023 12:32:26 +0100 Subject: [PATCH 4/9] add comments --- sentry_sdk/monitor.py | 7 +++++++ sentry_sdk/profiler.py | 11 ++++++++++- sentry_sdk/sessions.py | 7 +++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/sentry_sdk/monitor.py b/sentry_sdk/monitor.py index cf4aa20e86..71ca5e6c31 100644 --- a/sentry_sdk/monitor.py +++ b/sentry_sdk/monitor.py @@ -37,6 +37,13 @@ def __init__(self, transport, interval=10): def _ensure_running(self): # type: () -> None + """ + Check that the monitor has 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. + """ if self._thread_for_pid == os.getpid() and self._thread is not None: return None diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index 400767fd51..8e4d73af68 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -788,7 +788,8 @@ def ensure_running(self): def start_profiling(self, profile): # type: (Profile) -> None self.ensure_running() - self.new_profiles.append(profile) + if self.running: + self.new_profiles.append(profile) def stop_profiling(self, profile): # type: (Profile) -> None @@ -898,6 +899,14 @@ def teardown(self): def ensure_running(self): # type: () -> None + """ + Check that the profiler has an active thread to run in, and start one if + that's not the case. + + 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. + """ pid = os.getpid() # is running on the right process diff --git a/sentry_sdk/sessions.py b/sentry_sdk/sessions.py index f2aeb91265..68255184b7 100644 --- a/sentry_sdk/sessions.py +++ b/sentry_sdk/sessions.py @@ -105,6 +105,13 @@ def flush(self): 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. + """ if self._thread_for_pid == os.getpid() and self._thread is not None: return None with self._thread_lock: From 807f09f40fb945e86e9b596c31b28847e467ecf0 Mon Sep 17 00:00:00 2001 From: Ivana Kellyerova Date: Thu, 2 Nov 2023 12:54:34 +0100 Subject: [PATCH 5/9] revert condition --- sentry_sdk/profiler.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index 8e4d73af68..b3e4312cd4 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -788,8 +788,7 @@ def ensure_running(self): def start_profiling(self, profile): # type: (Profile) -> None self.ensure_running() - if self.running: - self.new_profiles.append(profile) + self.new_profiles.append(profile) def stop_profiling(self, profile): # type: (Profile) -> None From 30bf13ade296bedf51a42fc9b396bef02c7b3762 Mon Sep 17 00:00:00 2001 From: Ivana Kellyerova Date: Thu, 2 Nov 2023 13:17:25 +0100 Subject: [PATCH 6/9] add profiler test --- tests/test_profiler.py | 45 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/test_profiler.py b/tests/test_profiler.py index 451ebe65a3..866349792a 100644 --- a/tests/test_profiler.py +++ b/tests/test_profiler.py @@ -661,6 +661,51 @@ def test_thread_scheduler_single_background_thread(scheduler_class): assert len(get_scheduler_threads(scheduler)) == 0 +@requires_python_version(3, 3) +@pytest.mark.parametrize( + ("scheduler_class",), + [ + pytest.param(ThreadScheduler, id="thread scheduler"), + pytest.param( + GeventScheduler, + marks=[ + requires_gevent, + pytest.mark.skip( + reason="cannot find this thread via threading.enumerate()" + ), + ], + id="gevent scheduler", + ), + ], +) +def test_thread_scheduler_no_thread_on_shutdown(scheduler_class): + scheduler = scheduler_class(frequency=1000) + + # not yet setup, no scheduler threads yet + assert len(get_scheduler_threads(scheduler)) == 0 + + scheduler.setup() + + # setup but no profiles started so still no threads + assert len(get_scheduler_threads(scheduler)) == 0 + + # mock RuntimeError as if the 3.12 intepreter was shutting down + with mock.patch( + "threading.Thread.start", + side_effect=RuntimeError("can't create new thread at interpreter shutdown"), + ): + scheduler.ensure_running() + + assert scheduler.running is False + + # still no thread + assert len(get_scheduler_threads(scheduler)) == 0 + + scheduler.teardown() + + assert len(get_scheduler_threads(scheduler)) == 0 + + @requires_python_version(3, 3) @pytest.mark.parametrize( ("scheduler_class",), From 842950f0e3e60f209288180940f24c54fe63fd41 Mon Sep 17 00:00:00 2001 From: Ivana Kellyerova Date: Thu, 2 Nov 2023 13:22:40 +0100 Subject: [PATCH 7/9] add handling for gevent too --- sentry_sdk/profiler.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index b3e4312cd4..cd619f20d2 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -1018,7 +1018,13 @@ def ensure_running(self): self.running = True self.thread = ThreadPool(1) - self.thread.spawn(self.run) + try: + self.thread.spawn(self.run) + except RuntimeError: + # Unfortunately at this point the interpreter is in a state that no + # longer allows us to spawn a thread and we have to bail. + self.running = False + return def run(self): # type: () -> None From 3dcec2a17eb58e0644c9727b39e97bb9b1a01036 Mon Sep 17 00:00:00 2001 From: Ivana Kellyerova Date: Thu, 2 Nov 2023 13:50:09 +0100 Subject: [PATCH 8/9] more tests --- tests/test_monitor.py | 20 ++++++++++++++++++++ tests/test_sessions.py | 34 ++++++++++++++++++++++++++++++++++ tests/test_transport.py | 19 +++++++++++++++++++ 3 files changed, 73 insertions(+) diff --git a/tests/test_monitor.py b/tests/test_monitor.py index ec804ba513..42d600ebbb 100644 --- a/tests/test_monitor.py +++ b/tests/test_monitor.py @@ -3,6 +3,11 @@ from sentry_sdk import Hub, start_transaction from sentry_sdk.transport import Transport +try: + from unittest import mock # python 3.3 and above +except ImportError: + import mock # python < 3.3 + class HealthyTestTransport(Transport): def _send_event(self, event): @@ -82,3 +87,18 @@ def test_transaction_uses_downsampled_rate( assert transaction.sample_rate == 0.5 assert reports == [("backpressure", "transaction")] + + +def test_monitor_no_thread_on_shutdown_no_errors(sentry_init): + sentry_init(transport=HealthyTestTransport()) + + # make it seem like the interpreter is shutting down + with mock.patch( + "threading.Thread.start", + side_effect=RuntimeError("can't create new thread at interpreter shutdown"), + ): + monitor = Hub.current.client.monitor + assert monitor is not None + assert monitor._thread is None + monitor.run() + assert monitor._thread is None diff --git a/tests/test_sessions.py b/tests/test_sessions.py index 09b42b70a4..311aa53966 100644 --- a/tests/test_sessions.py +++ b/tests/test_sessions.py @@ -3,6 +3,11 @@ from sentry_sdk import Hub from sentry_sdk.sessions import auto_session_tracking +try: + from unittest import mock # python 3.3 and above +except ImportError: + import mock # python < 3.3 + def sorted_aggregates(item): aggregates = item["aggregates"] @@ -119,3 +124,32 @@ def test_aggregates_explicitly_disabled_session_tracking_request_mode( assert len(aggregates) == 1 assert aggregates[0]["exited"] == 1 assert "errored" not in aggregates[0] + + +def test_no_thread_on_shutdown_no_errors(sentry_init): + sentry_init( + release="fun-release", + environment="not-fun-env", + ) + + hub = Hub.current + + # make it seem like the interpreter is shutting down + with mock.patch( + "threading.Thread.start", + side_effect=RuntimeError("can't create new thread at interpreter shutdown"), + ): + with auto_session_tracking(session_mode="request"): + with sentry_sdk.push_scope(): + try: + raise Exception("all is wrong") + except Exception: + sentry_sdk.capture_exception() + + with auto_session_tracking(session_mode="request"): + pass + + hub.start_session(session_mode="request") + hub.end_session() + + sentry_sdk.flush() diff --git a/tests/test_transport.py b/tests/test_transport.py index 602f78437c..71c47e04fc 100644 --- a/tests/test_transport.py +++ b/tests/test_transport.py @@ -18,6 +18,10 @@ from sentry_sdk.envelope import Envelope, parse_json from sentry_sdk.integrations.logging import LoggingIntegration +try: + from unittest import mock # python 3.3 and above +except ImportError: + import mock # python < 3.3 CapturedData = namedtuple("CapturedData", ["path", "event", "envelope", "compressed"]) @@ -165,6 +169,21 @@ def test_transport_infinite_loop(capturing_server, request, make_client): assert len(capturing_server.captured) == 1 +def test_transport_no_thread_on_shutdown_no_errors(capturing_server, make_client): + client = make_client() + + # make it seem like the interpreter is shutting down + with mock.patch( + "threading.Thread.start", + side_effect=RuntimeError("can't create new thread at interpreter shutdown"), + ): + with Hub(client): + capture_message("hi") + + # nothing exploded but also no events can be sent anymore + assert len(capturing_server.captured) == 0 + + NOW = datetime(2014, 6, 2) From d4302a44a06911993662c5294fa72e69d4196c61 Mon Sep 17 00:00:00 2001 From: Ivana Kellyerova Date: Thu, 2 Nov 2023 14:11:00 +0100 Subject: [PATCH 9/9] also set thread to none --- sentry_sdk/profiler.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sentry_sdk/profiler.py b/sentry_sdk/profiler.py index cd619f20d2..8f90855b42 100644 --- a/sentry_sdk/profiler.py +++ b/sentry_sdk/profiler.py @@ -932,6 +932,7 @@ def ensure_running(self): # Unfortunately at this point the interpreter is in a state that no # longer allows us to spawn a thread and we have to bail. self.running = False + self.thread = None return def run(self): @@ -1024,6 +1025,7 @@ def ensure_running(self): # Unfortunately at this point the interpreter is in a state that no # longer allows us to spawn a thread and we have to bail. self.running = False + self.thread = None return def run(self):