GH-106176, GH-104702: Fix reference leak when importing across multiple threads by brettcannon · Pull Request #108497 · python/cpython · GitHub

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

…reads

!buildbot .*Windows11.*Refleaks

🤖 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

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.

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.

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

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!

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!

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

I would prefer to backport it to 3.12, because:

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

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)

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

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.

LGTM. Thanks for the update!

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

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

…cross multiple threads (pythonGH-108497) (cherry picked from commit 5f85b44) Co-authored-by: Brett Cannon <brett@python.org>

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

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

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

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

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
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.
Check if the failure is related to this commit (5f85b44) or if it is a false positive.
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_shutdown
    self.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

… multiple threads (GH-108497) (#108612) GH-106176, GH-104702: Fix reference leak when importing across multiple threads (GH-108497) (cherry picked from commit 5f85b44) Co-authored-by: Brett Cannon <brett@python.org>

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

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?

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.

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?

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

pythonGH-106176 : Fix reference leak when importing across multiple th…

50e4c5d

…reads

brettcannon requested review from ericsnowcurrently, ncoghlan and warsaw as code owners August 25, 2023 21:54

brettcannon removed request for ncoghlan and warsaw August 25, 2023 21:54

brettcannon self-assigned this Aug 25, 2023

brettcannon requested review from Eclips4 and vstinner and removed request for ericsnowcurrently August 25, 2023 21:54

bedevere-bot mentioned this pull request Aug 25, 2023

test_import.test_concurrency leaks ref on Windows #106176

Closed

bedevere-bot added the awaiting core review label Aug 25, 2023

brettcannon mentioned this pull request Aug 25, 2023

gh-106176: Fix reference leak in importlib/_bootstrap.py #106207

Closed

vstinner reviewed Aug 25, 2023

View reviewed changes

Comment thread Lib/importlib/_bootstrap.py Outdated

Comment thread Lib/importlib/_bootstrap.py Outdated

Comment thread Lib/importlib/_bootstrap.py Outdated

vstinner approved these changes Aug 25, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 25, 2023

Eclips4 approved these changes Aug 26, 2023

View reviewed changes

brettcannon added the needs backport to 3.12 only security fixes label Aug 28, 2023

brettcannon changed the title GH-106176: Fix reference leak when importing across multiple threads GH-106176, GH-104702: Fix reference leak when importing across multiple threads Aug 28, 2023

brettcannon added 2 commits August 28, 2023 16:39

Address reviewer comments

a2e94c3

Merge branch 'main' into issue-106176-_blocking_on-WeakValueDictionary

7d90811

vstinner approved these changes Aug 29, 2023

View reviewed changes

brettcannon added 3 commits August 28, 2023 17:49

Make a name very clearly private

432bea4

Merge branch 'main' into issue-106176-_blocking_on-WeakValueDictionary

c841af7

Merge branch 'main' into issue-106176-_blocking_on-WeakValueDictionary

78f5f7a

vstinner merged commit 5f85b44 into python:main Aug 29, 2023

bedevere-bot removed the awaiting merge label Aug 29, 2023

bedevere-bot removed the needs backport to 3.12 only security fixes label Aug 29, 2023

Eclips4 mentioned this pull request May 21, 2023

test_pickle are leaked on Windows #104702

Closed

Yhg1s reviewed Aug 29, 2023

View reviewed changes

brettcannon deleted the issue-106176-_blocking_on-WeakValueDictionary branch August 29, 2023 17:44

Eclips4 mentioned this pull request Jan 30, 2024

importlib/_bootstrap.py: Reduce size of _List instances #114747

Merged

-              # Dictionary protected by the global import lock
+              # For a list that can have a weakref to it.
+              class _List(list):
+                  pass

GH-106176, GH-104702: Fix reference leak when importing across multiple threads by brettcannon · Pull Request #108497 · python/cpython · GitHub

Uh oh!

Uh oh!

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

Conversation

brettcannon commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettcannon commented Aug 25, 2023

Uh oh!

bedevere-bot commented Aug 25, 2023

Uh oh!

Eclips4 commented Aug 25, 2023

Uh oh!

brettcannon commented Aug 25, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner commented Aug 25, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Eclips4 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eclips4 commented Aug 26, 2023

Uh oh!

Eclips4 commented Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettcannon commented Aug 28, 2023

Uh oh!

brettcannon commented Aug 28, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Aug 29, 2023

Uh oh!

bedevere-bot commented Aug 29, 2023

Uh oh!

vstinner commented Aug 29, 2023

Uh oh!

vstinner commented Aug 29, 2023

Uh oh!

bedevere-bot commented Aug 29, 2023

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

Uh oh!

Yhg1s Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettcannon Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

Yhg1s Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

Yhg1s commented Sep 18, 2023

Uh oh!

Yhg1s commented Sep 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

brettcannon commented Aug 25, 2023 •
edited
Loading

Eclips4 left a comment •
edited
Loading

Eclips4 commented Aug 27, 2023 •
edited
Loading

Yhg1s Aug 29, 2023 •
edited
Loading