Skip to content

Commit 979e756

Browse files
fix(profiling): fix silent suppression of AssertionError exceptions in _ProfiledLock::_acquire (#15089)
https://datadoghq.atlassian.net/browse/PROF-12903 ## Description `_ProfiledLock::_maybe_update_self_name` raises `AssertionError` in two places when `config.enable_asserts == True`. However, they're being silently suppressed by the try/catch block in the caller, `_ProfiledLock::_acquire`, method. Current estimated impact is low, as it's only ever triggered for development workflows, but still, worth making it right. This PR fixes the bug by propagating `AssertionError`'s by the caller. ## Testing Added two new tests: one for `AssertionError` and another for a non-`AssertionError` exception. **PROD** _Test fails, since the `AssertionError` exception is not propagated_ ``` $ ./scripts/ddtest riot -v run --pass-env d667cb4 -- -k test_assertion_error_suppressed_in_acquire ... FAILED tests/profiling_v2/collector/test_threading.py::TestThreadingLockCollector::test_assertion_error_suppressed_in_acquire - Failed: DID NOT RAISE <class 'AssertionError'> FAILED tests/profiling_v2/collector/test_threading.py::TestThreadingRLockCollector::test_assertion_error_suppressed_in_acquire - Failed: DID NOT RAISE <class 'AssertionError'> ``` _Highlighted code is for demonstration purposes only - not being merged_ <img width="703" height="735" alt="Screenshot 2025-10-30 at 10 36 28 AM" src="https://github.com/user-attachments/assets/4c3d8066-3aa2-465e-8386-0f13a3de0cf6" /> **BRANCH** _Test passes_ ``` $ ./scripts/ddtest riot -v run --pass-env d667cb4 -- -k test_assertion_error_suppressed_in_acquire ... tests/profiling_v2/collector/test_threading.py::TestThreadingLockCollector::test_assertion_error_suppressed_in_acquire PASSED tests/profiling_v2/collector/test_threading.py::TestThreadingRLockCollector::test_assertion_error_suppressed_in_acquire PASSED ``` ## Risks none [PROF-12903]: https://datadoghq.atlassian.net/browse/PROF-12903?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
1 parent 06ea45c commit 979e756

File tree

3 files changed

+86
-17
lines changed

3 files changed

+86
-17
lines changed

ddtrace/profiling/collector/_lock.py

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@
2929
from ddtrace.trace import Tracer
3030

3131

32+
ACQUIRE_RELEASE_CO_NAMES: frozenset[str] = frozenset(["_acquire", "_release"])
33+
ENTER_EXIT_CO_NAMES: frozenset[str] = frozenset(
34+
["acquire", "release", "__enter__", "__exit__", "__aenter__", "__aexit__"]
35+
)
36+
37+
3238
def _current_thread() -> Tuple[int, str]:
3339
thread_id: int = _thread.get_ident()
3440
return thread_id, _threading.get_thread_name(thread_id)
@@ -91,7 +97,12 @@ def _acquire(self, inner_func: Callable[..., Any], *args: Any, **kwargs: Any) ->
9197
try:
9298
self._maybe_update_self_name()
9399
self._flush_sample(start, end, is_acquire=True)
100+
except AssertionError:
101+
if config.enable_asserts:
102+
# AssertionError exceptions need to propagate
103+
raise
94104
except Exception:
105+
# Instrumentation must never crash user code
95106
pass # nosec
96107

97108
def release(self, *args: Any, **kwargs: Any) -> Any:
@@ -202,26 +213,16 @@ def _maybe_update_self_name(self) -> None:
202213
# 3: caller frame
203214
if config.enable_asserts:
204215
frame: FrameType = sys._getframe(1)
205-
# TODO: replace dict with list
206-
if frame.f_code.co_name not in {"_acquire", "_release"}:
207-
raise AssertionError("Unexpected frame %s" % frame.f_code.co_name)
216+
if frame.f_code.co_name not in ACQUIRE_RELEASE_CO_NAMES:
217+
raise AssertionError(f"Unexpected frame in stack: '{frame.f_code.co_name}'")
218+
208219
frame = sys._getframe(2)
209-
if frame.f_code.co_name not in {
210-
"acquire",
211-
"release",
212-
"__enter__",
213-
"__exit__",
214-
"__aenter__",
215-
"__aexit__",
216-
}:
217-
raise AssertionError("Unexpected frame %s" % frame.f_code.co_name)
218-
frame = sys._getframe(3)
220+
if frame.f_code.co_name not in ENTER_EXIT_CO_NAMES:
221+
raise AssertionError(f"Unexpected frame in stack: '{frame.f_code.co_name}'")
219222

220223
# First, look at the local variables of the caller frame, and then the global variables
221-
self._self_name = self._find_self_name(frame.f_locals) or self._find_self_name(frame.f_globals)
222-
223-
if not self._self_name:
224-
self._self_name = ""
224+
frame = sys._getframe(3)
225+
self._self_name = self._find_self_name(frame.f_locals) or self._find_self_name(frame.f_globals) or ""
225226

226227

227228
class FunctionWrapper(wrapt.FunctionWrapper):
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
profiling: This fix resolves an issue where AssertionError exceptions were silently suppressed in the `_acquire` method of the Lock profiler (note: this only occurs when assertions are enabled.)

tests/profiling_v2/collector/test_threading.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,70 @@ def validate_and_cleanup() -> None:
360360
validate_and_cleanup()
361361

362362

363+
@pytest.mark.subprocess(env=dict(DD_PROFILING_ENABLE_ASSERTS="true"))
364+
def test_assertion_error_raised_with_enable_asserts():
365+
"""Ensure that AssertionError is propagated when config.enable_asserts=True."""
366+
import threading
367+
368+
import mock
369+
import pytest
370+
371+
from ddtrace.internal.datadog.profiling import ddup
372+
from ddtrace.profiling.collector.threading import ThreadingLockCollector
373+
374+
# Initialize ddup (required before using collectors)
375+
assert ddup.is_available, "ddup is not available"
376+
ddup.config(env="test", service="test_asserts", version="1.0", output_filename="/tmp/test_asserts")
377+
ddup.start()
378+
379+
with ThreadingLockCollector(capture_pct=100):
380+
lock = threading.Lock()
381+
382+
# Patch _maybe_update_self_name to raise AssertionError
383+
lock._maybe_update_self_name = mock.Mock(side_effect=AssertionError("test: unexpected frame in stack"))
384+
385+
with pytest.raises(AssertionError):
386+
# AssertionError should be propagated when enable_asserts=True
387+
lock.acquire()
388+
389+
390+
@pytest.mark.subprocess(env=dict(DD_PROFILING_ENABLE_ASSERTS="false"))
391+
def test_all_exceptions_suppressed_by_default() -> None:
392+
"""
393+
Ensure that exceptions are silently suppressed in the `_acquire` method
394+
when config.enable_asserts=False (default).
395+
"""
396+
import threading
397+
398+
import mock
399+
400+
from ddtrace.internal.datadog.profiling import ddup
401+
from ddtrace.profiling.collector.threading import ThreadingLockCollector
402+
403+
# Initialize ddup (required before using collectors)
404+
assert ddup.is_available, "ddup is not available"
405+
ddup.config(env="test", service="test_exceptions", version="1.0", output_filename="/tmp/test_exceptions")
406+
ddup.start()
407+
408+
with ThreadingLockCollector(capture_pct=100):
409+
lock = threading.Lock()
410+
411+
# Patch _maybe_update_self_name to raise AssertionError
412+
lock._maybe_update_self_name = mock.Mock(side_effect=AssertionError("Unexpected frame in stack: 'fubar'"))
413+
lock.acquire()
414+
lock.release()
415+
416+
# Patch _maybe_update_self_name to raise RuntimeError
417+
lock._maybe_update_self_name = mock.Mock(side_effect=RuntimeError("Some profiling error"))
418+
lock.acquire()
419+
lock.release()
420+
421+
# Patch _maybe_update_self_name to raise Exception
422+
lock._maybe_update_self_name = mock.Mock(side_effect=Exception("Wut happened?!?!"))
423+
lock.acquire()
424+
lock.release()
425+
426+
363427
class BaseThreadingLockCollectorTest:
364428
# These should be implemented by child classes
365429
@property

0 commit comments

Comments
 (0)