bpo-44422: Fix threading.enumerate() reentrant call

[vstinner]

The threading.enumerate() function now uses a reentrant lock to
prevent a hang on reentrant call.

https://bugs.python.org/issue44422

[vstinner]

@pablogsal@pitrou@serhiy-storchaka@methane: I'm not sure about this change, would you mind to have a look?

[vstinner]

If the CI tests pass, I will try the buildbot label to check on more platforms.

[rhettinger]

The RLock() is the right way to go to prevent self-deadlock.

We could temporarily turn GC off but that seems hard to do in a way that reliably restores the prior enabled/disabled mode.

[vstinner]

We could temporarily turn GC off but that seems hard to do in a way that reliably restores the prior enabled/disabled mode.

Sure, that's the other option that I considered. But gc.disable() is process-wide: it affects all Python threads, and so it might have surprising side effects. Some code might rely on the current exact GC behavior.

Another option would be to rewrite the code in C. The problem is that other functions rely on this lock, like active_count(). I would prefer to not have to rewrite "half" of threading.py in C. Using a RLock is less intrusive.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 2907ca88719503493acd930790ffdb978e73dbbb 🤖

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

[pitrou]

(@iritkatriel : a concrete example of GC-induced reentrancy issue)

[vstinner]

I ran manual tests on this PR (my laptop has 8 logical CPUs, 4 physical Intel CPUs):

• Run ./python rec_threading.py: SUCCESS (rec_threading.py is attached to bpo-44422)
• Run ./python -m test -j10 -r twice: SUCCESS
• Run ./python -m test test_threading -F -j10 --timeout=60 for 15 minutes: SUCCESS (no timeout)

[vstinner]

Oops, there is a typo in a comment :-( "Use a reentrant lock to allocate reentrant calls": you should read "to allow". I will update my PR later, but I prefer to wait until the buildbot jobs complete.

[pitrou]

Indeed, a RLock seems like the best effort solution here. Presumably, only the non-mutating code protected by _active_limbo_lock can be called reentrantly (why would a reentrate call mutate the locks table?).

Still, a reminder that a lot of things can unfortunately happen in destructors and trigger reentrancy into unsuspecting code.

[vstinner]

(@iritkatriel : a concrete example of GC-induced reentrancy issue)

These bugs are surprising. Nobody expects the GC reentrancy! In OpenStack, it's even more surprising since threading.current_thread() is monkey-patched for even more fun!

[pitrou]

In OpenStack, it's even more surprising since threading.current_thread() is monkey-patched for even more fun!

Really? For what kind of fun? :-o

[vstinner]

Really? For what kind of fun? :-o

Python became too deterministic and boring. It's time to make it non deterministic again!

[vstinner]

9 Refleak builds failed:

• buildbot/AMD64 Fedora Stable Refleaks PR
• buildbot/AMD64 RHEL7 Refleaks PR
• buildbot/AMD64 RHEL8 Refleaks PR
• buildbot/PPC64LE Fedora Stable Refleaks PR
• buildbot/PPC64LE RHEL7 Refleaks PR
• buildbot/PPC64LE RHEL8 Refleaks PR
• buildbot/s390x Fedora Refleaks PR
• buildbot/s390x RHEL7 Refleaks PR
• buildbot/s390x RHEL8 Refleaks PR

test__xxsubinterpreters and/or test_threading failed:

test__xxsubinterpreters leaked [55, 55, 55] references, sum=165 test__xxsubinterpreters leaked [31, 31, 31] memory blocks, sum=93 ... test_threading leaked [110, 110, 110] references, sum=330 test_threading leaked [62, 62, 62] memory blocks, sum=186 

[iritkatriel]

The same lock is used in a few other places. Maybe in one of them it's not right to make it re-entrant?

[iritkatriel]

There is also another _active_limbo_lock = _allocate_lock() assignment in _after_fork:
https://github.com/python/cpython/blob/2907ca88719503493acd930790ffdb978e73dbbb/Lib/threading.py#L1562

[vstinner]

test__xxsubinterpreters and/or test_threading failed

Ah, the https://bugs.python.org/issue42972#msg385297 bug strikes back.

I wrote #26727 to fix the leak.

[vstinner]

There is also another _active_limbo_lock = _allocate_lock() assignment in _after_fork:

Oh, nicely stopped @iritkatriel! I will update the PR, once #26727 is merged.

[vstinner]

Ooops sorry, the PR to fix the leak is: #26734

[vstinner]

I fixed _after_fork() and the typo (allocate => allow), and I rebased it on top of the merged #26734 fix.

@iritkatriel: Would you mind to review the updated PR?

[iritkatriel]

This comment is now obsolete:

 # We don't call self._delete() because it also # grabs _active_limbo_lock. del _active[get_ident()] 

[vstinner]

This comment is now obsolete

Right @iritkatriel, I plan to write a second change only for that, but only change it in the main branch to avoid any risk of regression in stable branches. Oh I forgot to mention it here, I have a local patch for it ;-)

So @iritkatriel, does it look good to you?

[iritkatriel]

LGTM

[miss-islington]

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

[bedevere-bot]

GH-26737 is a backport of this pull request to the 3.10 branch.

[bedevere-bot]

GH-26738 is a backport of this pull request to the 3.9 branch.

[vstinner]

Thanks everyone for your useful reviews ;-) I wasn't confident at all that this change was safe. At least, the responsibility of any possible regression is now distributed :-D

[vstinner]

This comment is now obsolete: (...)

@iritkatriel: I created #26741 to use again the _delete() method in _bootstrap_inner().