fix(profiling): Ensure profiler thread exits when needed

The soft exit wasn't properly shutting down the thread if another profiler
started up too quickly. This ensures it is reused if possible but is properly
shutdown if needed.

Specifically, the shutdown allowed the profiler 1 cycle before actually shutting
down. If another profiler is started during this cycle, it's possible the old
profiler never shuts down. Resulting in multiple profilers running.

Author: Zylphrex

sentry_sdk/profiler/continuous_profiler.py

@@ -236,6 +236,7 @@ def __init__(self, frequency, options, sdk_info, capture_func):
         self.pid = None  # type: Optional[int]
 
         self.running = False
+        self.soft_shutdown = False
 
         self.new_profiles = deque(maxlen=128)  # type: Deque[ContinuousProfile]
         self.active_profiles = set()  # type: Set[ContinuousProfile]
@@ -317,24 +318,23 @@ def profiler_id(self):
         return self.buffer.profiler_id
 
     def make_sampler(self):
-        # type: () -> Callable[..., None]
+        # type: () -> Callable[..., bool]
         cwd = os.getcwd()
 
         cache = LRUCache(max_size=256)
 
         if self.lifecycle == "trace":
 
             def _sample_stack(*args, **kwargs):
-                # type: (*Any, **Any) -> None
+                # type: (*Any, **Any) -> bool
                 """
                 Take a sample of the stack on all the threads in the process.
                 This should be called at a regular interval to collect samples.
                 """
 
                 # no profiles taking place, so we can stop early
                 if not self.new_profiles and not self.active_profiles:
-                    self.running = False
-                    return
+                    return True
 
                 # This is the number of profiles we want to pop off.
                 # It's possible another thread adds a new profile to
@@ -357,7 +357,7 @@ def _sample_stack(*args, **kwargs):
                     # For some reason, the frame we get doesn't have certain attributes.
                     # When this happens, we abandon the current sample as it's bad.
                     capture_internal_exception(sys.exc_info())
-                    return
+                    return False
 
                 # Move the new profiles into the active_profiles set.
                 #
@@ -374,9 +374,7 @@ def _sample_stack(*args, **kwargs):
                 inactive_profiles = []
 
                 for profile in self.active_profiles:
-                    if profile.active:
-                        pass
-                    else:
+                    if not profile.active:
                         # If a profile is marked inactive, we buffer it
                         # to `inactive_profiles` so it can be removed.
                         # We cannot remove it here as it would result
@@ -389,10 +387,12 @@ def _sample_stack(*args, **kwargs):
                 if self.buffer is not None:
                     self.buffer.write(ts, sample)
 
+                return False
+
         else:
 
             def _sample_stack(*args, **kwargs):
-                # type: (*Any, **Any) -> None
+                # type: (*Any, **Any) -> bool
                 """
                 Take a sample of the stack on all the threads in the process.
                 This should be called at a regular interval to collect samples.
@@ -409,19 +409,21 @@ def _sample_stack(*args, **kwargs):
                     # For some reason, the frame we get doesn't have certain attributes.
                     # When this happens, we abandon the current sample as it's bad.
                     capture_internal_exception(sys.exc_info())
-                    return
+                    return False
 
                 if self.buffer is not None:
                     self.buffer.write(ts, sample)
 
+                return False
+
         return _sample_stack
 
     def run(self):
         # type: () -> None
         last = time.perf_counter()
 
         while self.running:
-            self.sampler()
+            self.soft_shutdown = self.sampler()
 
             # some time may have elapsed since the last time
             # we sampled, so we need to account for that and
@@ -430,6 +432,15 @@ def run(self):
             if elapsed < self.interval:
                 thread_sleep(self.interval - elapsed)
 
+            # the soft shutdown happens here to give it a chance
+            # for the profiler to be reused
+            if self.soft_shutdown:
+                self.running = False
+
+                # make sure to explicitly exit the profiler here or there might
+                # be multiple profilers at once
+                break
+
             # after sleeping, make sure to take the current
             # timestamp so we can use it next iteration
             last = time.perf_counter()
@@ -458,6 +469,8 @@ def __init__(self, frequency, options, sdk_info, capture_func):
     def ensure_running(self):
         # type: () -> None
 
+        self.soft_shutdown = False
+
         pid = os.getpid()
 
         # is running on the right process
@@ -532,6 +545,9 @@ def __init__(self, frequency, options, sdk_info, capture_func):
 
     def ensure_running(self):
         # type: () -> None
+
+        self.soft_shutdown = False
+
         pid = os.getpid()
 
         # is running on the right process

tests/profiler/test_continuous_profiler.py

@@ -459,33 +459,54 @@ def test_continuous_profiler_auto_start_and_stop_sampled(
 
     thread = threading.current_thread()
 
+    all_profiler_ids = set()
+
     for _ in range(3):
         envelopes.clear()
 
+        profiler_ids = set()
+
         with sentry_sdk.start_transaction(name="profiling 1"):
-            assert get_profiler_id() is not None, "profiler should be running"
+            profiler_id = get_profiler_id()
+            assert profiler_id is not None, "profiler should be running"
+            profiler_ids.add(profiler_id)
             with sentry_sdk.start_span(op="op"):
                 time.sleep(0.1)
-            assert get_profiler_id() is not None, "profiler should be running"
+            profiler_id = get_profiler_id()
+            assert profiler_id is not None, "profiler should be running"
+            profiler_ids.add(profiler_id)
+
+        time.sleep(0.03)
 
         # the profiler takes a while to stop in auto mode so if we start
         # a transaction immediately, it'll be part of the same chunk
-        assert get_profiler_id() is not None, "profiler should be running"
+        profiler_id = get_profiler_id()
+        assert profiler_id is not None, "profiler should be running"
+        profiler_ids.add(profiler_id)
 
         with sentry_sdk.start_transaction(name="profiling 2"):
-            assert get_profiler_id() is not None, "profiler should be running"
+            profiler_id = get_profiler_id()
+            assert profiler_id is not None, "profiler should be running"
+            profiler_ids.add(profiler_id)
             with sentry_sdk.start_span(op="op"):
                 time.sleep(0.1)
-            assert get_profiler_id() is not None, "profiler should be running"
+            profiler_id = get_profiler_id()
+            assert profiler_id is not None, "profiler should be running"
+            profiler_ids.add(profiler_id)
 
         # wait at least 1 cycle for the profiler to stop
         time.sleep(0.2)
         assert get_profiler_id() is None, "profiler should not be running"
 
+        assert len(profiler_ids) == 1
+        all_profiler_ids.add(profiler_ids.pop())
+
         assert_single_transaction_with_profile_chunks(
             envelopes, thread, max_chunks=1, transactions=2
         )
 
+    assert len(all_profiler_ids) == 3
+
 
 @pytest.mark.parametrize(
     "mode",