Skip to content

Commit 89824c3

Browse files
Yun-Kimbrettlangdon
authored andcommitted
fix(profiler): decrement refcount given strong reference of frame object in python 3.11 (#4901)
## Description #4125 added the use of a getter function `PyThread_GetFrame()` to provide compatibility for Python 3.11 in the profiler (specifically the stack collector). However, `PyThread_GetFrame()` returns a strong reference to a `PyFrameObject` and its reference count was not decremented in that change. This means that the frame object will not be garbage collected and likely the cause behind the memory leak mentioned in #4899. ## Checklist - [ ] Followed the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines) when writing a release note. - [ ] Add additional sections for `feat` and `fix` pull requests. - [ ] [Library documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs) and/or [Datadog's documentation site](https://github.com/DataDog/documentation/) is updated. Link to doc PR in description. <!-- Copy and paste the relevant snippet based on the type of pull request --> <!-- START feat --> ## Motivation <!-- Expand on why the change is required, include relevant context for reviewers --> ## Design <!-- Include benefits from the change as well as possible drawbacks and trade-offs --> ## Testing strategy <!-- Describe the automated tests and/or the steps for manual testing. <!-- END feat --> <!-- START fix --> ## Relevant issue(s) <!-- Link the pull request to any issues related to the fix. Use keywords for links to automate closing the issues once the pull request is merged. --> ## Testing strategy <!-- Describe any added regression tests and/or the manual testing performed. --> <!-- END fix --> ## Reviewer Checklist - [ ] Title is accurate. - [ ] Description motivates each change. - [ ] No unnecessary changes were introduced in this PR. - [ ] Avoid breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [ ] Tests provided or description of manual testing performed is included in the code or PR. - [ ] Release note has been added and follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines), or else `changelog/no-changelog` label added. - [ ] All relevant GitHub issues are correctly linked. - [ ] Backports are identified and tagged with Mergifyio. Co-authored-by: Brett Langdon <[email protected]>
1 parent 6cdc26f commit 89824c3

File tree

3 files changed

+30
-2
lines changed

3 files changed

+30
-2
lines changed

ddtrace/profiling/collector/stack.pyx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,7 @@ cdef collect_threads(thread_id_ignore_list, thread_time, thread_span_links) with
257257
if exc_tb:
258258
current_exceptions[tstate.thread_id] = (<object>exc_type, <object>exc_tb)
259259
Py_XDECREF(exc_tb)
260+
Py_XDECREF(frame)
260261
ELSE:
261262
frame = tstate.frame
262263
if frame:
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 in Python 3.11 where a PyFrameObject strong reference count was not properly
5+
decremented in the stack collector.

tests/profiling/collector/test_stack.py

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
# -*- encoding: utf-8 -*-
22
import collections
3+
import gc
34
import os
45
import sys
56
import threading
67
import time
78
import timeit
9+
from types import FrameType
810
import typing
911
import uuid
1012

@@ -400,7 +402,7 @@ def test_exception_collection():
400402
assert e.sampling_period > 0
401403
assert e.thread_id == nogevent.thread_get_ident()
402404
assert e.thread_name == "MainThread"
403-
assert e.frames == [(__file__, 394, "test_exception_collection", "")]
405+
assert e.frames == [(__file__, 396, "test_exception_collection", "")]
404406
assert e.nframes == 1
405407
assert e.exc_type == ValueError
406408

@@ -432,7 +434,7 @@ def test_exception_collection_trace(
432434
assert e.sampling_period > 0
433435
assert e.thread_id == nogevent.thread_get_ident()
434436
assert e.thread_name == "MainThread"
435-
assert e.frames == [(__file__, 421, "test_exception_collection_trace", "")]
437+
assert e.frames == [(__file__, 423, "test_exception_collection_trace", "")]
436438
assert e.nframes == 1
437439
assert e.exc_type == ValueError
438440
assert e.span_id == span.span_id
@@ -743,3 +745,23 @@ def _nothing():
743745
# assert (exact_time * 0.7) <= values.pop() <= (exact_time * 1.3)
744746

745747
assert values.pop() > 0
748+
749+
750+
@pytest.mark.skipif(sys.version_info < (3, 11, 0), reason="PyFrameObjects are lazy-created objects in Python 3.11+")
751+
def test_collect_ensure_all_frames_gc():
752+
# Regression test for memory leak with lazy PyFrameObjects in Python 3.11+
753+
def _foo():
754+
pass
755+
756+
r = recorder.Recorder()
757+
s = stack.StackCollector(r)
758+
759+
with s:
760+
for _ in range(100):
761+
_foo()
762+
763+
gc.collect() # Make sure we don't race with gc when we check frame objects
764+
frametype_objs = [obj for obj in gc.get_objects() if isinstance(obj, FrameType)]
765+
for obj in frametype_objs:
766+
# Ensure that all frames relating to _foo() have been gc'd already
767+
assert obj.f_code.co_name != "_foo"

0 commit comments

Comments
 (0)