From c9d71321b919bf8ecba9b1ac7835af5686bd48e1 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 5 Aug 2024 19:02:58 +0000 Subject: [PATCH 1/8] gh-122697: Fix free-threading memory leaks at shutdown We were not properly accounting for interpreter memory leaks at shutdown and had two sources of leaks: * Objects that use deferred reference counting and were reachable via static types outlive the final GC. We now disable deferred reference counting on all objects if we are calling the GC due to interpreter shutdown. * `_PyMem_FreeDelayed` did not properly check for interpreter shutdown so we had some memory blocks that were enqueued to be freed, but never actually freed. --- Objects/obmalloc.c | 8 +++++--- Python/gc_free_threading.c | 12 ++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/Objects/obmalloc.c b/Objects/obmalloc.c index a6a71802ef8e01..3d40595f8959fe 100644 --- a/Objects/obmalloc.c +++ b/Objects/obmalloc.c @@ -1109,9 +1109,9 @@ free_delayed(uintptr_t ptr) #ifndef Py_GIL_DISABLED free_work_item(ptr); #else - if (_PyRuntime.stoptheworld.world_stopped) { - // Free immediately if the world is stopped, including during - // interpreter shutdown. + if (_PyRuntime.stoptheworld.world_stopped || Py_IsFinalizing()) { + // Free immediately if the world is stopped and during interpreter + // shutdown. free_work_item(ptr); return; } @@ -1474,6 +1474,8 @@ _PyInterpreterState_FinalizeAllocatedBlocks(PyInterpreterState *interp) { #ifdef WITH_MIMALLOC if (_PyMem_MimallocEnabled()) { + Py_ssize_t leaked = _PyInterpreterState_GetAllocatedBlocks(interp); + interp->runtime->obmalloc.interpreter_leaks += leaked; return; } #endif diff --git a/Python/gc_free_threading.c b/Python/gc_free_threading.c index 53f04160c38841..aefff46435514d 100644 --- a/Python/gc_free_threading.c +++ b/Python/gc_free_threading.c @@ -54,6 +54,7 @@ struct collection_state { struct visitor_args base; PyInterpreterState *interp; GCState *gcstate; + _PyGC_Reason reason; Py_ssize_t collected; Py_ssize_t uncollectable; Py_ssize_t long_lived_total; @@ -571,6 +572,16 @@ scan_heap_visitor(const mi_heap_t *heap, const mi_heap_area_t *area, worklist_push(&state->unreachable, op); } } + else if (state->reason == _Py_GC_REASON_SHUTDOWN && + _PyObject_HasDeferredRefcount(op)) + { + // Disable deferred refcounting for reachable objects as well during + // interpreter shutdown. This ensures that these objects are collected + // immediately when their last reference is removed. + disable_deferred_refcounting(op); + merge_refcount(op, 0); + state->long_lived_total++; + } else { // object is reachable, restore `ob_tid`; we're done with these objects gc_restore_tid(op); @@ -1221,6 +1232,7 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason) struct collection_state state = { .interp = interp, .gcstate = gcstate, + .reason = reason, }; gc_collect_internal(interp, &state, generation); From 04f958286eedb7ba23b9615fcd2000630fc6d178 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 5 Aug 2024 19:28:44 +0000 Subject: [PATCH 2/8] Add blurb --- .../2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst new file mode 100644 index 00000000000000..6c312055204c30 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst @@ -0,0 +1,2 @@ +Fixed memory leaks at interpreter shutdown in the free-threaded build, and +also reporting of leaked memory blocks via ``-X showrefcount``. From 28d67681053c41ce0cd49bb42f92367979a03ab3 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Mon, 5 Aug 2024 19:34:31 +0000 Subject: [PATCH 3/8] Adjust comment --- Objects/obmalloc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Objects/obmalloc.c b/Objects/obmalloc.c index 3d40595f8959fe..8af985c5adc81d 100644 --- a/Objects/obmalloc.c +++ b/Objects/obmalloc.c @@ -1109,9 +1109,9 @@ free_delayed(uintptr_t ptr) #ifndef Py_GIL_DISABLED free_work_item(ptr); #else - if (_PyRuntime.stoptheworld.world_stopped || Py_IsFinalizing()) { - // Free immediately if the world is stopped and during interpreter - // shutdown. + if (Py_IsFinalizing() || _PyRuntime.stoptheworld.world_stopped) { + // Free immediately during interpreter shutdown or if the world is + // stopped. free_work_item(ptr); return; } From 7fc200bf0c4416bd05b14d7d1508b69168595095 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 7 Aug 2024 15:11:39 +0000 Subject: [PATCH 4/8] Use interpreter shutdown instead of runtime shutdown --- Objects/obmalloc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Objects/obmalloc.c b/Objects/obmalloc.c index 8af985c5adc81d..dfeccfa4dd7658 100644 --- a/Objects/obmalloc.c +++ b/Objects/obmalloc.c @@ -1109,7 +1109,10 @@ free_delayed(uintptr_t ptr) #ifndef Py_GIL_DISABLED free_work_item(ptr); #else - if (Py_IsFinalizing() || _PyRuntime.stoptheworld.world_stopped) { + PyInterpreterState *interp = _PyInterpreterState_GET(); + if (_PyInterpreterState_GetFinalizing(interp) != NULL || + interp->stoptheworld.world_stopped) + { // Free immediately during interpreter shutdown or if the world is // stopped. free_work_item(ptr); From 05db2d8fbfefb4f870e78b66c83d2e359baf44aa Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 7 Aug 2024 14:42:42 -0400 Subject: [PATCH 5/8] Update Misc/NEWS.d/next/Core_and_Builtins/2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst Co-authored-by: Kumar Aditya --- .../2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst index 6c312055204c30..34ee6a916bcf33 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-08-05-19-28-12.gh-issue-122697.17MvYl.rst @@ -1,2 +1,2 @@ Fixed memory leaks at interpreter shutdown in the free-threaded build, and -also reporting of leaked memory blocks via ``-X showrefcount``. +also reporting of leaked memory blocks via :option:`-X showrefcount <-X>`. From 33cc40caf0085d925d1c57ee2da89eea53409526 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 7 Aug 2024 19:26:12 +0000 Subject: [PATCH 6/8] Add missing call to _PyType_FinalizeIdPool --- Python/pylifecycle.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 6b641c0775f533..7e305579f1e33c 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1832,6 +1832,7 @@ finalize_interp_types(PyInterpreterState *interp) _PyTypes_FiniTypes(interp); _PyTypes_Fini(interp); + _PyType_FinalizeIdPool(interp); _PyCode_Fini(interp); From 312f8605dab2589a85e2436dfb32eb33b151c0b3 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 7 Aug 2024 19:31:55 +0000 Subject: [PATCH 7/8] Fix includes --- Python/pylifecycle.c | 1 + Python/pystate.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 7e305579f1e33c..28c712ad5e8b66 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -28,6 +28,7 @@ #include "pycore_sliceobject.h" // _PySlice_Fini() #include "pycore_sysmodule.h" // _PySys_ClearAuditHooks() #include "pycore_traceback.h" // _Py_DumpTracebackThreads() +#include "pycore_typeid.h" // _PyType_FinalizeIdPool() #include "pycore_typeobject.h" // _PyTypes_InitTypes() #include "pycore_typevarobject.h" // _Py_clear_generic_types() #include "pycore_unicodeobject.h" // _PyUnicode_InitTypes() diff --git a/Python/pystate.c b/Python/pystate.c index 8f4818cee00d9d..bba88b76088e71 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -20,7 +20,7 @@ #include "pycore_runtime_init.h" // _PyRuntimeState_INIT #include "pycore_sysmodule.h" // _PySys_Audit() #include "pycore_obmalloc.h" // _PyMem_obmalloc_state_on_heap() -#include "pycore_typeid.h" // _PyType_FinalizeIdPool +#include "pycore_typeid.h" // _PyType_FinalizeThreadLocalRefcounts() /* -------------------------------------------------------------------------- CAUTION From 70e25f577d0a726206e0ead5def93cc16455457a Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 7 Aug 2024 19:36:13 +0000 Subject: [PATCH 8/8] Add missing Py_GIL_DISABLED ifdef guard --- Python/pylifecycle.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 28c712ad5e8b66..3a41c640fd9599 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -1833,7 +1833,9 @@ finalize_interp_types(PyInterpreterState *interp) _PyTypes_FiniTypes(interp); _PyTypes_Fini(interp); +#ifdef Py_GIL_DISABLED _PyType_FinalizeIdPool(interp); +#endif _PyCode_Fini(interp);