GH-106176, GH-104702: Fix reference leak when importing across multiple threads

[brettcannon]

When importing, import deadlock detection tracks which threads are importing which modules in a list. Prior to this change, a list was created per thread but never disposed of. Now, the list is disposed of properly using weakrefs to guarantee GC doesn't interrupt the cleanup.

Co-Authored-By: Kirill Podoprigora kirill.bast9@mail.ru

• Issue: test_import.test_concurrency leaks ref on Windows #106176
• Issue: test_pickle are leaked on Windows #104702

[brettcannon]

!buildbot .*Windows11.*Refleaks

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @brettcannon for commit 50e4c5d 🤖

The command will test the builders whose names match following regular expression: .*Windows11.*Refleaks

The builders matched are:

• AMD64 Windows11 Refleaks PR

[Eclips4]

This is probably what we should have done.
I'll take a closer look later today (UTC+3).
However, I'd like to hear @exarkun opinion if he has free time.

[brettcannon]

FYI the Windows 11 refleak builder has not come back yet, so the green CI is a little misleading until we can get confirmation this PR doesn't leak under Windows.

[vstinner]

I'm now curious if it does fix the issue.

[vstinner]

I'm still able to reproduce the leak in the main branch:

vstinner@WIN C:\victor\python\main>python -m test test_import -m test_concurrency -R 3:3
test_import leaked [38, 36, 40] references, sum=114
test_import leaked [38, 36, 40] memory blocks, sum=114

And with this PR: this is no leak, it works!

vstinner@WIN C:\victor\python\main>python -m test test_import -m test_concurrency -R 3:3
Running Debug|x64 interpreter...
0:00:00 Run tests sequentially
0:00:00 [1/1] test_import
beginning 6 repetitions
123456
......
== Tests result: SUCCESS ==
1 test OK.
Total duration: 7.7 sec
Tests result: SUCCESS

Well done @brettcannon and @Eclips4!

[vstinner]

LGTM since it does fix the leak :-) I left a review, feel free to address it, or ignore my remarks ;-) Thanks for fixing Windows Refleaks buildbots!

[Eclips4]

+1 to all of Victor's words.
LGTM!
(I've check it on my windows setup and this solution seems fine, refleaks are gone)

[Eclips4]

I would prefer to backport it to 3.12, because:

1. Original issue was introduced in 3.12
2. This problem looks like one of real situations which can happen.

[Eclips4]

This also solves the #104702.
(and that's wonderful!)
For explanation why test_pickle is also affected by this problem, see my comment: #106207 (comment)

[brettcannon]

Thanks for the reviews and verification! I should be able to get this merged at worst by Friday, but hopefully sooner.

[brettcannon]

I addressed the review comments from Victor, updated against main, flagged this for 3.12, and now I'm waiting for CI to go green again before I merge.

[vstinner]

LGTM. Thanks for the update!

[miss-islington]

Thanks @brettcannon for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[bedevere-bot]

GH-108612 is a backport of this pull request to the 3.12 branch.

[vstinner]

@brettcannon:

I addressed the review comments from Victor, updated against main, flagged this for 3.12, and now I'm waiting for CI to go green again before I merge.

Thanks. I took the freedom of merging your PR. It became difficult to read buildbot results these days, since there are more and more known failures. This fix should reduce the number of failed buildbots!

[vstinner]

Again, thanks for the fix @brettcannon and @Eclips4!

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot aarch64 Fedora Stable LTO 3.x has failed when building commit 5f85b44.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/336/builds/3857) and take a look at the build logs.
4. Check if the failure is related to this commit (5f85b44) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/336/builds/3857

Failed tests:

• test.test_concurrent_futures.test_shutdown

Failed subtests:

• test_interpreter_shutdown - test.test_concurrent_futures.test_shutdown.ProcessPoolSpawnProcessPoolShutdownTest.test_interpreter_shutdown

Summary of the results of the build (if available):

== Tests result: FAILURE then FAILURE ==

448 tests OK.

10 slowest tests:

• test_gdb: 4 min 17 sec
• test.test_multiprocessing_spawn.test_processes: 1 min 14 sec
• test.test_concurrent_futures.test_wait: 1 min 12 sec
• test_socket: 1 min 10 sec
• test_signal: 1 min
• test_subprocess: 56.7 sec
• test.test_multiprocessing_forkserver.test_processes: 52.9 sec
• test_io: 44.7 sec
• test.test_multiprocessing_spawn.test_misc: 43.5 sec
• test.test_asyncio.test_tasks: 41.3 sec

1 test failed:
test.test_concurrent_futures.test_shutdown

14 tests skipped:
test.test_asyncio.test_windows_events
test.test_asyncio.test_windows_utils test_devpoll test_ioctl
test_kqueue test_launcher test_startfile test_tkinter test_ttk
test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test.test_concurrent_futures.test_shutdown

Total duration: 4 min 44 sec

Click to see traceback logs

Traceback (most recent call last):
File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/test/test_concurrent_futures/test_shutdown.py", line 49, in test_interpreter_shutdownself.assertFalse(err)
AssertionError: b'Exception in thread Thread-1:\nTraceback (most recent call last):\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 1059, in _bootstrap_inner\n self.run()\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/concurrent/futures/process.py", line 339, in run\n self.add_call_item_to_queue()\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/concurrent/futures/process.py", line 394, in add_call_item_to_queue\n self.call_queue.put(_CallItem(work_id,\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/queues.py", line 94, in put\n self._start_thread()\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/queues.py", line 177, in _start_thread\n self._thread.start()\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/threading.py", line 978, in start\n _start_new_thread(self._bootstrap, ())\nRuntimeError: can\'t create new thread at interpreter shutdown\nTraceback (most recent call last):\n File "<string>", line 1, in <module>\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/spawn.py", line 122, in spawn_main\n exitcode = _main(fd, parent_sentinel)\n ^^^^^^^^^^^^^^^^^^^^^^^^^^\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/spawn.py", line 132, in _main\n self = reduction.pickle.load(from_parent)\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-aarch64.lto/build/Lib/multiprocessing/synchronize.py", line 115, in __setstate__\n self._semlock = _multiprocessing.SemLock._rebuild(*state)\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\nFileNotFoundError: [Errno 2] No such file or directory\n' is not false

[Yhg1s]

FWIW, I think it would be nice to use __slots__ for this class, to reduce the size of each instance (especially since the setdefault call creates one each time, even when it isn't necessary).

[brettcannon]

As in you're going to make a PR to make that change, or you're hoping someone else will open an issue on your behalf to track the idea?

[Yhg1s]

The latter (although not on my behalf) -- I don't know if there's a reason not to do it, and I don't have the spare cycles to dig in and find out.

[Yhg1s]

FWIW, this does not seem to have resolved the refleak in test_import in 3.12 (see e.g. https://buildbot.python.org/all/#/builders/1125/builds/115) even though it does seem to have fixed them in main. Does anyone want to take a look at why test_import is still leaking in 3.12?

[Yhg1s]

Nevermind, the leak was from a different issue (still not resolved on main, but more cleverly hidden).