From a1ea2fe8078071d410fda4c6702e1fec15f406c2 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 22 Jan 2025 18:14:31 +0100 Subject: [PATCH 1/6] gh-129185: Fix PyTraceMalloc_Untrack() at Python exit Support calling PyTraceMalloc_Track() and PyTraceMalloc_Untrack() during late Python finalization. --- Lib/test/test_tracemalloc.py | 25 +++++++++++++++++++++++++ Python/tracemalloc.c | 10 ++++++++++ 2 files changed, 35 insertions(+) diff --git a/Lib/test/test_tracemalloc.py b/Lib/test/test_tracemalloc.py index 238ae14b388c76..02b448724f3d65 100644 --- a/Lib/test/test_tracemalloc.py +++ b/Lib/test/test_tracemalloc.py @@ -1,6 +1,7 @@ import contextlib import os import sys +import textwrap import tracemalloc import unittest from unittest.mock import patch @@ -19,6 +20,7 @@ _testinternalcapi = None +DEFAULT_DOMAIN = 0 EMPTY_STRING_SIZE = sys.getsizeof(b'') INVALID_NFRAME = (-1, 2**30) @@ -1110,6 +1112,29 @@ def test_tracemalloc_track_race(self): # gh-128679: Test fix for tracemalloc.stop() race condition _testcapi.tracemalloc_track_race() + def test_late_untrack(self): + code = textwrap.dedent(f""" + from test import support + import tracemalloc + import _testcapi + + class Tracked: + def __init__(self, domain, size): + self.domain = domain + self.ptr = id(self) + self.size = size + _testcapi.tracemalloc_track(self.domain, self.ptr, self.size) + + def __del__(self, untrack=_testcapi.tracemalloc_untrack): + untrack(self.domain, self.ptr) + + domain = {DEFAULT_DOMAIN} + tracemalloc.start() + obj = Tracked(domain, 1024 * 1024) + support.late_deletion(obj) + """) + assert_python_ok("-c", code) + if __name__ == "__main__": unittest.main() diff --git a/Python/tracemalloc.c b/Python/tracemalloc.c index b398ea379e0607..318386ead64705 100644 --- a/Python/tracemalloc.c +++ b/Python/tracemalloc.c @@ -1255,6 +1255,11 @@ int PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr, size_t size) { + // gh-129185: Pre-check to support calls after _PyTraceMalloc_Fini() + if (!tracemalloc_config.tracing) { + return -2; + } + PyGILState_STATE gil_state = PyGILState_Ensure(); TABLES_LOCK(); @@ -1277,6 +1282,11 @@ PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr, int PyTraceMalloc_Untrack(unsigned int domain, uintptr_t ptr) { + // gh-129185: Pre-check to support calls after _PyTraceMalloc_Fini() + if (!tracemalloc_config.tracing) { + return -2; + } + TABLES_LOCK(); int result; From 4172b9140af9ee8ce757bf059d2ab29affef53a8 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 23 Jan 2025 02:41:34 +0100 Subject: [PATCH 2/6] Test also PyTraceMalloc_Untrack() without the GIL --- Lib/test/test_tracemalloc.py | 20 +++++++++++++------- Modules/_testcapi/mem.c | 13 +++++++++++-- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/Lib/test/test_tracemalloc.py b/Lib/test/test_tracemalloc.py index 02b448724f3d65..0220a83d24b428 100644 --- a/Lib/test/test_tracemalloc.py +++ b/Lib/test/test_tracemalloc.py @@ -1029,8 +1029,8 @@ def track(self, release_gil=False, nframe=1): release_gil) return frames - def untrack(self): - _testcapi.tracemalloc_untrack(self.domain, self.ptr) + def untrack(self, release_gil=False): + _testcapi.tracemalloc_untrack(self.domain, self.ptr, release_gil) def get_traced_memory(self): # Get the traced size in the domain @@ -1072,7 +1072,7 @@ def test_track_already_tracked(self): self.assertEqual(self.get_traceback(), tracemalloc.Traceback(frames)) - def test_untrack(self): + def check_untrack(self, release_gil): tracemalloc.start() self.track() @@ -1080,13 +1080,19 @@ def test_untrack(self): self.assertEqual(self.get_traced_memory(), self.size) # untrack must remove the trace - self.untrack() + self.untrack(release_gil) self.assertIsNone(self.get_traceback()) self.assertEqual(self.get_traced_memory(), 0) # calling _PyTraceMalloc_Untrack() multiple times must not crash - self.untrack() - self.untrack() + self.untrack(release_gil) + self.untrack(release_gil) + + def test_untrack(self): + self.check_untrack(False) + + def test_untrack_without_gil(self): + self.check_untrack(True) def test_stop_track(self): tracemalloc.start() @@ -1126,7 +1132,7 @@ def __init__(self, domain, size): _testcapi.tracemalloc_track(self.domain, self.ptr, self.size) def __del__(self, untrack=_testcapi.tracemalloc_untrack): - untrack(self.domain, self.ptr) + untrack(self.domain, self.ptr, 1) domain = {DEFAULT_DOMAIN} tracemalloc.start() diff --git a/Modules/_testcapi/mem.c b/Modules/_testcapi/mem.c index ab4ad934644c38..ecae5ba26226a6 100644 --- a/Modules/_testcapi/mem.c +++ b/Modules/_testcapi/mem.c @@ -557,8 +557,9 @@ tracemalloc_untrack(PyObject *self, PyObject *args) { unsigned int domain; PyObject *ptr_obj; + int release_gil = 0; - if (!PyArg_ParseTuple(args, "IO", &domain, &ptr_obj)) { + if (!PyArg_ParseTuple(args, "IO|i", &domain, &ptr_obj, &release_gil)) { return NULL; } void *ptr = PyLong_AsVoidPtr(ptr_obj); @@ -566,7 +567,15 @@ tracemalloc_untrack(PyObject *self, PyObject *args) return NULL; } - int res = PyTraceMalloc_Untrack(domain, (uintptr_t)ptr); + int res; + if (release_gil) { + Py_BEGIN_ALLOW_THREADS + res = PyTraceMalloc_Untrack(domain, (uintptr_t)ptr); + Py_END_ALLOW_THREADS + } + else { + res = PyTraceMalloc_Untrack(domain, (uintptr_t)ptr); + } if (res < 0) { PyErr_SetString(PyExc_RuntimeError, "PyTraceMalloc_Untrack error"); return NULL; From f3ec0958e78e7485542cfa639ba60fdeb31e4e48 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 23 Jan 2025 02:43:07 +0100 Subject: [PATCH 3/6] Call _PyTraceMalloc_Fini() after finalize_interp_clear() --- Python/pylifecycle.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index f6526725d5dccc..52890cfc5df829 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2113,7 +2113,7 @@ _Py_Finalize(_PyRuntimeState *runtime) /* Disable tracemalloc after all Python objects have been destroyed, so it is possible to use tracemalloc in objects destructor. */ - _PyTraceMalloc_Fini(); + _PyTraceMalloc_Stop(); /* Finalize any remaining import state */ // XXX Move these up to where finalize_modules() is currently. @@ -2166,6 +2166,8 @@ _Py_Finalize(_PyRuntimeState *runtime) finalize_interp_clear(tstate); + _PyTraceMalloc_Fini(); + #ifdef Py_TRACE_REFS /* Display addresses (& refcnts) of all objects still alive. * An address can be used to find the repr of the object, printed From d510567c32bf52044351c7a68df3acef5eb12e4c Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 23 Jan 2025 02:43:17 +0100 Subject: [PATCH 4/6] PyTraceMalloc_Track() checks tracing with the GIL --- Python/tracemalloc.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/Python/tracemalloc.c b/Python/tracemalloc.c index 318386ead64705..fda138201f3940 100644 --- a/Python/tracemalloc.c +++ b/Python/tracemalloc.c @@ -1255,15 +1255,17 @@ int PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr, size_t size) { - // gh-129185: Pre-check to support calls after _PyTraceMalloc_Fini() + PyGILState_STATE gil_state = PyGILState_Ensure(); + // gh-129185: Check before TABLES_LOCK() to support calls after + // _PyTraceMalloc_Fini(). + int result; if (!tracemalloc_config.tracing) { - return -2; + result = -2; + goto unlock_gil; } - PyGILState_STATE gil_state = PyGILState_Ensure(); TABLES_LOCK(); - int result; if (tracemalloc_config.tracing) { result = tracemalloc_add_trace_unlocked(domain, ptr, size); } @@ -1273,6 +1275,7 @@ PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr, } TABLES_UNLOCK(); +unlock_gil: PyGILState_Release(gil_state); return result; @@ -1282,7 +1285,9 @@ PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr, int PyTraceMalloc_Untrack(unsigned int domain, uintptr_t ptr) { - // gh-129185: Pre-check to support calls after _PyTraceMalloc_Fini() + // gh-129185: Check before TABLES_LOCK() to support calls after + // _PyTraceMalloc_Fini(). This check is prone to race if another thread + // calls _PyTraceMalloc_Fini() in parallel. if (!tracemalloc_config.tracing) { return -2; } From 31bfe5b98c5d2607f5530bac8e29a185e3c1d49f Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 23 Jan 2025 02:57:29 +0100 Subject: [PATCH 5/6] Test also Untrack in test_tracemalloc_track_race() --- Modules/_testcapimodule.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 996b96bc000450..c405a352ed74a1 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -3394,6 +3394,7 @@ static void tracemalloc_track_race_thread(void *data) { PyTraceMalloc_Track(123, 10, 1); + PyTraceMalloc_Untrack(123, 10); PyThread_type_lock lock = (PyThread_type_lock)data; PyThread_release_lock(lock); From 555c70dd1f02f509f12389b5328083b23b0ba8b4 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 23 Jan 2025 03:02:37 +0100 Subject: [PATCH 6/6] Get the GIL in Untrack --- Python/tracemalloc.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/Python/tracemalloc.c b/Python/tracemalloc.c index fda138201f3940..62065e85ac8350 100644 --- a/Python/tracemalloc.c +++ b/Python/tracemalloc.c @@ -1256,12 +1256,13 @@ PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr, size_t size) { PyGILState_STATE gil_state = PyGILState_Ensure(); + int result; + // gh-129185: Check before TABLES_LOCK() to support calls after // _PyTraceMalloc_Fini(). - int result; if (!tracemalloc_config.tracing) { result = -2; - goto unlock_gil; + goto done; } TABLES_LOCK(); @@ -1275,7 +1276,7 @@ PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr, } TABLES_UNLOCK(); -unlock_gil: +done: PyGILState_Release(gil_state); return result; @@ -1285,16 +1286,19 @@ PyTraceMalloc_Track(unsigned int domain, uintptr_t ptr, int PyTraceMalloc_Untrack(unsigned int domain, uintptr_t ptr) { + // Need the GIL to prevent races on the first 'tracing' test + PyGILState_STATE gil_state = PyGILState_Ensure(); + int result; + // gh-129185: Check before TABLES_LOCK() to support calls after - // _PyTraceMalloc_Fini(). This check is prone to race if another thread - // calls _PyTraceMalloc_Fini() in parallel. + // _PyTraceMalloc_Fini() if (!tracemalloc_config.tracing) { - return -2; + result = -2; + goto done; } TABLES_LOCK(); - int result; if (tracemalloc_config.tracing) { tracemalloc_remove_trace_unlocked(domain, ptr); result = 0; @@ -1305,6 +1309,8 @@ PyTraceMalloc_Untrack(unsigned int domain, uintptr_t ptr) } TABLES_UNLOCK(); +done: + PyGILState_Release(gil_state); return result; }