fix(profiling): wrap del with try/except #12833

taegyunkim · 2025-03-21T11:13:25Z

        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

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

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
The change includes or references documentation updates if necessary
Backport labels are set (if applicable)

Reviewer Checklist

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 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

github-actions · 2025-03-21T11:14:05Z

CODEOWNERS have been resolved as:

releasenotes/notes/profiling-self-acquired-at-963cadba71ac62b8.yaml     @DataDog/apm-python
ddtrace/profiling/collector/_lock.py                                    @DataDog/profiling-python

github-actions · 2025-03-21T11:32:28Z

Bootstrap import analysis

Comparison of import times between this PR and main.

Summary

The average import time in this PR is: 236 ± 3 ms.

The average import time in main is: 238 ± 3 ms.

The import time difference between this PR and main is: -1.3 ± 0.1 ms.

Import time breakdown

The following import paths have grown:

ddtrace.auto 0.021 ms (0.01%)

ddtrace.bootstrap.sitecustomize 0.021 ms (0.01%)

ddtrace.bootstrap.preload 0.021 ms (0.01%)

ddtrace.internal.products 0.021 ms (0.01%)

ddtrace.settings.dynamic_instrumentation 0.021 ms (0.01%)

The following import paths have shrunk:

ddtrace.auto 1.892 ms (0.80%)

ddtrace.bootstrap.sitecustomize 1.249 ms (0.53%)

ddtrace.bootstrap.preload 1.249 ms (0.53%)

ddtrace.internal.products 1.249 ms (0.53%)

ddtrace.internal.remoteconfig.client 0.623 ms (0.26%)

ddtrace 0.643 ms (0.27%)

pr-commenter · 2025-03-21T11:57:25Z

Benchmarks

Benchmark execution time: 2025-03-21 16:21:08

Comparing candidate commit 536b9b5 in PR branch taegyunkim/del-acquired-at with baseline commit 00c254b in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 498 metrics, 2 unstable metrics.

ddtrace/profiling/collector/_lock.py

@nsrip-dd

```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) (cherry picked from commit 5c8fb46)

@nsrip-dd

```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) (cherry picked from commit 5c8fb46)

@nsrip-dd

```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) (cherry picked from commit 5c8fb46)

@nsrip-dd

```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) (cherry picked from commit 5c8fb46)

@nsrip-dd

Backport 5c8fb46 from #12833 to 2.21. ```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 <taegyun.kim@datadoghq.com>

@nsrip-dd

Backport 5c8fb46 from #12833 to 3.3. ```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 <taegyun.kim@datadoghq.com>

@nsrip-dd

Backport 5c8fb46 from #12833 to 3.2. ```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 <taegyun.kim@datadoghq.com>

@nsrip-dd

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 <taegyun.kim@datadoghq.com>

taegyunkim added 2 commits March 21, 2025 06:56

fix(profiling): wrap del with try/except

bc090f0

release note

bc8ff07

taegyunkim requested review from a team as code owners March 21, 2025 11:13

taegyunkim requested review from erikayasuda and juanjux March 21, 2025 11:13

taegyunkim added the Profiling Continous Profling label Mar 21, 2025

nsrip-dd reviewed Mar 21, 2025

View reviewed changes

ddtrace/profiling/collector/_lock.py Outdated Show resolved Hide resolved

restructure and add comments

536b9b5

nsrip-dd approved these changes Mar 21, 2025

View reviewed changes

taegyunkim merged commit 5c8fb46 into main Mar 21, 2025
325 of 328 checks passed

taegyunkim deleted the taegyunkim/del-acquired-at branch March 21, 2025 16:33

taegyunkim added backport 2.21 [DEPRECATED] backport 3.1 [DEPRECATED] backport 3.2 backport 3.3 labels Mar 21, 2025

taegyunkim mentioned this pull request Mar 25, 2025

fix(profiling): avoid change-after-check pattern for lock #12792

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(profiling): wrap del with try/except #12833

fix(profiling): wrap del with try/except #12833

Uh oh!

taegyunkim commented Mar 21, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Mar 21, 2025

Uh oh!

github-actions bot commented Mar 21, 2025 •

edited

Loading

Uh oh!

pr-commenter bot commented Mar 21, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(profiling): wrap del with try/except #12833

fix(profiling): wrap del with try/except #12833

Uh oh!

Conversation

taegyunkim commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Reviewer Checklist

Uh oh!

github-actions bot commented Mar 21, 2025

Uh oh!

github-actions bot commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bootstrap import analysis

Summary

Import time breakdown

Uh oh!

pr-commenter bot commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

taegyunkim commented Mar 21, 2025 •

edited

Loading

github-actions bot commented Mar 21, 2025 •

edited

Loading

pr-commenter bot commented Mar 21, 2025 •

edited

Loading