I can confirm this PR fixes thegh-132413 issue as well.

I think this fixes (or mostly fixes)gh-91636 as well.

🤖 New build scheduled with the buildbot fleet by@nascheme for commit12f0b5c 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136189%2Fmerge

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

This introduces refleaks, it seems. One of the leaking tests:
test.test_concurrent_futures.test_shutdown.ProcessPoolSpawnProcessPoolShutdownTest.test_shutdown_gh_132969_case_1
My unconfirmed suspicion is that a finalizer is now resurrecting an object via a weakref. Previously that weakref would be cleared before the finalizer is run. The multiprocessing finalizer logic seems very complicated. :-/

This is the smallest leaking example I could make so far. Something withProcessPoolExecutor leaking maybe.

class LeakTest(unittest.TestCase):    @classmethod    def _fail_task(cls, n):        raise ValueError("failing task")    def test_leak_case(self):        # this leaks references        executor = futures.ProcessPoolExecutor(                max_workers=1,                max_tasks_per_child=1,                )        f2 = executor.submit(LeakTest._fail_task, 0)        try:            f2.result()        except ValueError:            pass        # Ensure that the executor cleans up after called        # shutdown with wait=False        executor_manager_thread = executor._executor_manager_thread        executor.shutdown(wait=False)        time.sleep(0.2)        executor_manager_thread.join()    def test_leak_case2(self):        # this does not leak        with futures.ProcessPoolExecutor(                max_workers=1,                max_tasks_per_child=1,                ) as executor:            f2 = executor.submit(LeakTest._fail_task, 0)            try:                f2.result()            except ValueError:                pass

Other leaking examples (on Windows):

importloggingimportlogging.configimportlogging.handlersfrommultiprocessingimportQueue,ManagerclassConfigDictTest(unittest.TestCase):deftest_multiprocessing_queues_XXX(self):config= {'version':1,'handlers' : {'spam' : {'class':'logging.handlers.QueueHandler','queue':Manager().Queue()  ,# Leak# 'queue': Manager().JoinableQueue()  # Leak# 'queue': Queue(),                   # No leak                },            },'root': {'handlers': ['spam']}        }logger=logging.getLogger()logging.config.dictConfig(config)whilelogger.handlers:h=logger.handlers[0]logger.removeHandler(h)h.close()

importcontextlibimportthreadingimporttypesfromconcurrentimportinterpretersdeffunc():raiseException('spam!')@contextlib.contextmanagerdefcaptured_thread_exception():ctx=types.SimpleNamespace(caught=None)defexcepthook(args):ctx.caught=argsorig_excepthook=threading.excepthookthreading.excepthook=excepthooktry:yieldctxfinally:threading.excepthook=orig_excepthookclassTestInterpreterCall(unittest.TestCase):deftest_call_in_thread_XXX(self):interp=interpreters.create()call= (interp._call,interp.call)[1]# 0: No leak, 1: Leakwithcaptured_thread_exception()as_:t=threading.Thread(target=call,args=(interp,func, (), {}))t.start()t.join()

importweakref,_interpreterswd=weakref.WeakValueDictionary()classTestInterpreterCall(unittest.TestCase):deftest_call_in_thread_XXX(self):id=_interpreters.create()wd[id]=type("", (), {})_interpreters.destroy(id)

The majority (maybe all) of these leaks are caused by theWeakValueDictionary used asmultiprocessing.util._afterfork_registry. That took some digging to find. I'm not yet sure of a good fix for this. Explicitly cleaning the dead weak references from the.data dict works but it not too elegant.

The majority (maybe all) of these leaks are caused by the WeakValueDictionary used as multiprocessing.util._afterfork_registry. That took some digging to find. I'm not yet sure of a good fix for this. Explicitly cleaning the dead weak references from the .data dict works but it not too elegant.

Nope, that doesn't fix all the leaks. And having to explicitly clean the weakrefs from the WeakValueDictionary really shouldn't be needed, I think. TheKeyedRef class uses a callback and so they should be cleaned from the dict when the referred value dies. So, I'm not exactly sure what's going on there.

For the purposes of having a fix that we can backport (should probably be backported to all maintained Python versions), a less disruptive fix would be better. To that end, I've changed this PR to only defer clearing weakrefs to class objects. That fixes thetp_subclasses bug but should be less likely to break currently working code.

🤖 New build scheduled with the buildbot fleet by@nascheme for commit2f3daba 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F136189%2Fmerge

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

The KeyedRef class uses a callback and so they should be cleaned from the dict when the referred value dies. So, I'm not exactly sure what's going on there.

Ah, theKeyedRef callback requires that the weakref is cleared when it is called, otherwise it was not deleting the item from the weak dictionary. So we need to clear the weakrefs with callbacks before executing them. That fixes the refleaks, I believe.

I revised this PR to be something that is potentially suitable for backporting. To minimize the behaviour change, I'm only deferring the clear of weakrefs that refer to types (in order to allow tp_subclasses to work) or with callbacks. I still have an extra clearing pass that gets done after the finalizers are called. That avoids bugs like#91636.

If this gets merged, I think we should consider deferring clearing all weakrefs (without callbacks) until after finalizers are executed. I think that's more correct since it gives the finalizers a better opportunity to do their job.

TheKeyedRef class uses a callback and so they should be cleaned from the dict when the referred value dies. So, I'm not exactly sure what's going on there.

IIUC, there maybe cases whenselfref orself_weakref already cleared byWeakValueDictionary or_WeakValueDictionary still have values should should be cleared to. But they will not be cleared because of conditionif selfref() is not None andif self_weakref() is not None.

IIUC, becausePEP-442 landed we can remove weakrefs here

and there

and use a strong reference that will outlive the KeyedRefs in the dictionaries. I replaced the weakref with a strong reference in the _bootstrap.py and checked with refleak buildbot (#136345). No leaks were found.

@nascheme WDYT?

Weakrefwith a callback needs to be cleared early (as-is), accounting for the 3rd-party applications that have similar logic toWeakValueDictionary.

Weakrefwith a callback needs to be cleared early (as-is)

I'm not against this. But IIUC (feel free to correct me) we can face a situation in which weakref to theWeakValueDictionary is cleared in onegc collect call andKeyedRefs are cleared in another. And strong ref should handle this situation. We can teach users about this (I suppose).

we can face a situation

The experiment#136345 seems currently invalid, which is based on main and not effective on this PR where I omitted calling_PyWeakref_ClearRef inhandle_weakref_callbacks.

The experiment#136345 seems currently invalid, which is based on main and not effective on this PR where I omitted calling_PyWeakref_ClearRef inhandle_weakref_callbacks.

If we use a strong reference, then it doesn't matter if we omit_PyWeakref_ClearRef or not. Am I wrong?

Before discussing, have you checked on this PR? I can see the leaks.

Let's move on to#136345. (The result above looks long here.)

GH-136401 was merged. I think it should be backported as well since it should be pretty low risk.

The main branch has been merged into this PR. I've updated the PR to defer clearing all weakrefs without callbacks (not just weakrefs to classes). This seems more correct to me (rather than special casing weakrefs to classes) and seems the best way to fixGH-132413.

I thinkGH-132413 is actually more likely to be encountered in real code. For example, if you have some logging function in a__del__ method that uses the datetime module. So, maybe this PR should be backported to 3.13 and 3.14 as well.

Looks good to me. Thanks for detailed comments!

@pablogsal are you planning to take another look at this? If so, no problem, there is no great rush. If not, I think I'll do another review pass myself and then merge.

Yeah i have it pending for this week (sorry fixing a lot of bugs for 3.14 currently) but if you feel I am taking too much time I am happy if you go ahead I don't want to block it on this

@nascheme I see you updated comment forhandle_weakrefs and removed mentions ofGC_REACHABLE andGC_TENTATIVELY_UNREACHABLE.

Would you mind also removing another one fromsubtract_refs comment?

Do we want to merge this PR w/o tests on subclasses destruction from#135552?
Or maybe it will be better to place them here?

Do we want to merge this PR w/o tests on subclasses destruction from#135552?
Or maybe it will be better to place them here?

Sergey has tests in another PR and I'll merge that right after I merge this one.