GeezerVison

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

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

🤖 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.

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

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)

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.

(@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!

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

Really? For what kind of fun? :-o

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

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

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

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

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.

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.

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

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?

This comment is now obsolete:

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

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?

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

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

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

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

This comment is now obsolete: (...)

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