Skip to content

Commit 59f01d6

Browse files
fix(profiling): wrap del with try/except [backport 3.1] (#12841)
Backport 5c8fb46 from #12833 to 3.1. ```Python start = None if hasattr(self, "_self_acquired_at"): # _self_acquired_at is only set when the acquire was captured # if it's not set, we're not capturing the release start = self._self_acquired_at del self._self_acquired_at ``` Above code can raise an `AttributeError`, if there are multiple threads calling on `release()`. Though in such scenarios, except for the thread which actually held and released the lock, such threads would result in a `RuntimeError`, the current behavior makes customers blame our code instead of theirs. The following code, by @nsrip-dd, can be used to check whether the code raises an error or not ```Python import threading import sys sys.setswitchinterval(0.0000001) def unlock(l): try: l.release() except RuntimeError: pass except Exception as e: raise e while True: l = threading.Lock() l.acquire() threads = [threading.Thread(target=unlock, args=[l,]) for _ in range(64)] for t in threads: t.start() for t in threads: t.join() ``` ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) Co-authored-by: Taegyun Kim <[email protected]>
1 parent d7b40c9 commit 59f01d6

File tree

2 files changed

+20
-6
lines changed

2 files changed

+20
-6
lines changed

ddtrace/profiling/collector/_lock.py

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -180,13 +180,22 @@ def acquire(self, *args, **kwargs):
180180
def _release(self, inner_func, *args, **kwargs):
181181
# type (typing.Any, typing.Any) -> None
182182

183-
start = None
184-
if hasattr(self, "_self_acquired_at"):
185-
# _self_acquired_at is only set when the acquire was captured
186-
# if it's not set, we're not capturing the release
187-
start = self._self_acquired_at
183+
# The underlying threading.Lock class is implemented using C code, and
184+
# it doesn't have the __dict__ attribute. So we can't do
185+
# self.__dict__.pop("_self_acquired_at", None) to remove the attribute.
186+
# Instead, we need to use the following workaround to retrieve and
187+
# remove the attribute.
188+
start = getattr(self, "_self_acquired_at", None)
189+
try:
190+
# Though it should generally be avoided to call release() from
191+
# multiple threads, it is possible to do so. In that scenario, the
192+
# following statement code will raise an AttributeError. This should
193+
# not be propagated to the caller and to the users. The inner_func
194+
# will raise an RuntimeError as the threads are trying to release()
195+
# and unlocked lock, and the expected behavior is to propagate that.
188196
del self._self_acquired_at
189-
197+
except AttributeError:
198+
LOG.debug("Failed to delete _self_acquired_at")
190199
try:
191200
return inner_func(*args, **kwargs)
192201
finally:
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
fixes:
3+
- |
4+
profiling: This fix resolves an issue where the Lock profiler would throw
5+
an ``AttributeError: '_ProfiledThreadingLock' object has no attribute '_self_acquired_at'``.

0 commit comments

Comments
 (0)