gh-90549: Fix leak of global named resources using multiprocessing spawn #30617

jxdabc · 2022-01-15T18:38:05Z

Repo is the standard tool that Google uses to build Android, Chrome OS, Chromium, etc, written in Python.

Many Repo users have encountered resource leak warnings with Python 3.9.
https://bugs.chromium.org/p/gerrit/issues/detail?id=14934&q=component%3Arepo&can=5

I did some work and found that the problem is not caused by the code of Repo, but a bug of the Python library, multiprocess.

To make it simple, the Python script below leaks named resource even when exiting normally. (And be unlinked by resource_tracker with a warning. )

import multiprocessing as mp

global_resource = mp.Semaphore()

def submain(): pass

if __name__ == '__main__':
    p = mp.Process(target=submain)
    p.start()
    p.join()

Tested on macOS with Python 3.9.7

python test.py
resource_tracker: There appear to be 1 leaked semaphore objects to clean up at shutdown.

This bug will 100% reproduce when then main module uses named resources as global variables and uses spawn context, which is the case of Repo on macOS.

This is caused by multiprocess::BaseProcess::_bootstrap.

When a new process is started with multiprocessing.Process.start() in spawn context.

The main module is reloaded in the subprocess (for pickle) in multiprocessing::spawn::_main.
Named resources (such as the semaphore above) in the main module resister their _cleanup into multiprocessing::util::_finalizer_registry, which unlink themselves.
multiprocess::BaseProcess::_bootstrap then clears _finalizer_registry.

When a subprocess is spawned, it is no need to clear util::_finalizer_registry (and no need to call util::_run_after_forkers). Disable clearing _finalizer_registry (and disable call to _run_after_forkers) should fix this bug without breaking anything else.

And I uploaded a MR.

https://bugs.python.org/issue46391

the-knights-who-say-ni · 2022-01-15T18:38:08Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@jxdabc

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

jxdabc · 2022-01-26T16:59:09Z

cc @pitrou

jxdabc · 2022-02-01T09:06:04Z

@1st1

arhadthedev · 2022-06-02T08:20:52Z

Could you merge a fresh main into your branch? The tests are stuck so nobody can even see if anything is broken.

jxdabc · 2022-06-06T02:17:07Z

Could you merge a fresh main into your branch? The tests are stuck so nobody can even see if anything is broken.

OK. I've rebased the patch to the main.

jxdabc · 2022-06-06T03:17:42Z

Could you merge a fresh main into your branch? The tests are stuck so nobody can even see if anything is broken.

@arhadthedev Hope for review and fix. The problem caused by this bug is really annoying. A warning appears on most of the repo commands I use. #90549

arhadthedev · 2022-06-06T04:20:30Z

cc @pitrou and @1st1 once again

pitrou

Thanks for diagnosing this and finding a fix! Here are a couple comments.

Lib/test/_test_multiprocessing.py

Lib/multiprocessing/context.py

Lib/multiprocessing/process.py

bedevere-bot · 2022-06-06T10:46:06Z

🤖 New build scheduled with the buildbot fleet by @pitrou for commit 11b3674851836dfc0cd1a1817e26b625a95bb32b 🤖

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

Lib/test/_test_multiprocessing.py

Lib/multiprocessing/context.py

Lib/test/_test_multiprocessing.py

Lib/multiprocessing/context.py

Lib/test/_test_multiprocessing.py

jxdabc · 2022-06-09T08:04:31Z

@pitrou Anything else I could do to get this fixed?

LGTM.

@pitrou: I let you merge it ;-)

bedevere-bot · 2022-06-09T12:04:12Z

🤖 New build scheduled with the buildbot fleet by @pitrou for commit 7cc22ae 🤖

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

pitrou

+1, thanks a lot @jxdabc .

pitrou · 2022-06-09T12:06:57Z

Let's wait a bit for the buildbot results now.

pitrou · 2022-06-09T12:08:48Z

We might want to backport this to 3.10, given that this fixes a leak. multiprocessing is however a complex library and there is a non-zero risk that this would cause unforeseen regressions.

jxdabc · 2022-06-09T12:27:19Z

We might want to backport this to 3.10, given that this fixes a leak. multiprocessing is however a complex library and there is a non-zero risk that this would cause unforeseen regressions.

@pitrou what I need to do about this backport?

pitrou · 2022-06-09T12:31:59Z

@jxdabc Nothing special, it's a decision core developers make.

@vstinner @pablogsal What do you think?

vstinner · 2022-06-09T12:48:22Z

It's a bugfix, it should be backported.

there is a non-zero risk that this would cause unforeseen regressions.

That's right, but if it introduces a regression, it can be fixed as well.

jxdabc · 2022-06-09T14:45:47Z

Let's wait a bit for the buildbot results now.

@pitrou Glad to see all check passed.

miss-islington · 2022-06-09T16:55:15Z

Thanks @jxdabc for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

bedevere-bot · 2022-06-09T16:55:23Z

GH-93651 is a backport of this pull request to the 3.11 branch.

bedevere-bot · 2022-06-09T16:55:27Z

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

jxdabc requested a review from 1st1 as a code owner January 15, 2022 18:38

the-knights-who-say-ni added the CLA not signed label Jan 15, 2022

bedevere-bot added the awaiting review label Jan 15, 2022

jxdabc force-pushed the fix-issue-46391 branch 2 times, most recently from 3aef095 to 055a4c2 Compare January 16, 2022 05:02

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jan 18, 2022

jxdabc force-pushed the fix-issue-46391 branch from fa9d895 to dd3fca1 Compare January 26, 2022 16:52

smithdc1 mentioned this pull request Feb 15, 2022

Fixed #31169 -- Adapted the parallel test runner to use spawn. django/django#15421

Merged

jxdabc mannequin mentioned this pull request Apr 20, 2022

Library multiprocess leaks named resources. #90549

Closed

jxdabc force-pushed the fix-issue-46391 branch from dd3fca1 to d8161cb Compare June 5, 2022 22:49

pitrou reviewed Jun 6, 2022

View reviewed changes

Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved

Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved

Lib/multiprocessing/context.py Outdated Show resolved Hide resolved

Lib/multiprocessing/process.py Outdated Show resolved Hide resolved

pitrou added topic-multiprocessing 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Jun 6, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 6, 2022

jxdabc force-pushed the fix-issue-46391 branch from 11b3674 to e9acebd Compare June 7, 2022 03:58

jxdabc commented Jun 7, 2022

View reviewed changes

Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved

jxdabc commented Jun 7, 2022

View reviewed changes

Lib/multiprocessing/context.py Outdated Show resolved Hide resolved

jxdabc commented Jun 7, 2022

View reviewed changes

Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved

jxdabc commented Jun 7, 2022

View reviewed changes

Lib/multiprocessing/context.py Outdated Show resolved Hide resolved

jxdabc requested a review from pitrou June 7, 2022 04:26

pitrou reviewed Jun 7, 2022

View reviewed changes

Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved

jxdabc force-pushed the fix-issue-46391 branch from e6f8e28 to 7cc22ae Compare June 9, 2022 11:58

pitrou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 9, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 9, 2022

pitrou approved these changes Jun 9, 2022

View reviewed changes

pitrou changed the title ~~gh-90549: Library multiprocess leaks named resources.~~ gh-90549: Fix leak of global named resources using multiprocessing spawn Jun 9, 2022

pitrou added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Jun 9, 2022

pitrou merged commit 30610d2 into python:main Jun 9, 2022

bedevere-bot removed the awaiting merge label Jun 9, 2022

bedevere-bot removed the needs backport to 3.11 only security fixes label Jun 9, 2022

bedevere-bot removed the needs backport to 3.10 only security fixes label Jun 9, 2022

jxdabc deleted the fix-issue-46391 branch June 9, 2022 23:16

chrissimpkins mentioned this pull request Jun 17, 2022

resource_tracker UserWarning on crunch executable processing with multi-file requests (parallel execution) chrissimpkins/Crunch#100

Open

vindex10 mentioned this pull request Aug 7, 2024

multiprocessing.Pool: leak of global named resources using multiprocessing spawn #122770

Open

Uh oh!

gh-90549: Fix leak of global named resources using multiprocessing spawn #30617

gh-90549: Fix leak of global named resources using multiprocessing spawn #30617

Uh oh!

Conversation

jxdabc commented Jan 15, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Jan 15, 2022

CLA Missing

Uh oh!

jxdabc commented Jan 26, 2022

Uh oh!

jxdabc commented Feb 1, 2022

Uh oh!

arhadthedev commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jxdabc commented Jun 6, 2022

Uh oh!

jxdabc commented Jun 6, 2022

Uh oh!

arhadthedev commented Jun 6, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jun 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jxdabc commented Jun 9, 2022

Uh oh!

bedevere-bot commented Jun 9, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jun 9, 2022

Uh oh!

pitrou commented Jun 9, 2022

Uh oh!

jxdabc commented Jun 9, 2022

Uh oh!

pitrou commented Jun 9, 2022

Uh oh!

vstinner commented Jun 9, 2022

Uh oh!

jxdabc commented Jun 9, 2022

Uh oh!

miss-islington commented Jun 9, 2022

Uh oh!

bedevere-bot commented Jun 9, 2022

Uh oh!

bedevere-bot commented Jun 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jxdabc commented Jan 15, 2022 •

edited by bedevere-bot

Loading

arhadthedev commented Jun 2, 2022 •

edited

Loading