gh-109593: Fix reentrancy issue in multiprocessing resource_tracker #109629
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
pitrou
commented
Sep 20, 2023
•
pitrou
commented
- Issue: test_multiprocessing_forkserver.test_processes: ResourceTracker.ensure_running() calls itself and hangs indirectly via the GC #109593
|
@vstinner This solution should be robust while avoiding the performance issue of calling |
pitrou
added
🔨 test-with-buildbots
bedevere-bot
removed
🔨 test-with-refleak-buildbots
vstinner
reviewed
Sep 21, 2023
vstinner
reviewed
Sep 21, 2023
Should we? I don't want to enter a discussion of whether this private API is generally useful. It is purely meant to allow solving this issue. |
vstinner
approved these changes
Sep 22, 2023
pitrou
force-pushed
the
gh109593-ensure-running-reentrant
branch
from
7c01743 to
8471da5
Compare
pitrou
requested review from
brettcannon,
ericsnowcurrently,
ncoghlan and
warsaw
as code owners
pitrou
commented
Sep 22, 2023
| warnings.warn( | ||
| f"ResourceTracker called reentrantly for resource cleanup, " | ||
| f"which is unsupported. " | ||
| f"The {rtype} object {name!r} might leak.") |
There was a problem hiding this comment.
@vstinner This might raise a warning from time to time during the test suite, will it fail the buildbots?
There was a problem hiding this comment.
Hum. If you run tests with -Werror, it will raises an UserWarning exception, and can make the tests fail, no?
|
@vstinner I did some minor changes (mostly, I added a warning). Do you want to take another look? |
The default timeout test of test_threading is failing for many years: issue gh-109401. I launched a new job. |
|
@vstinner So are you ok with this PR after all? I can't think of another backportable solution. A cleaner solution may be found for the development version, but it will be too risky to backport. |
vstinner
reviewed
Sep 26, 2023
| @@ -81,6 +97,9 @@ def ensure_running(self): | |||
| This can be run from any process. Usually a child process will use | |||
| the resource created by its parent.''' | |||
| with self._lock: | |||
| if self._lock._recursion_count() > 1: | |||
| # The code below is certainly not reentrant-safe, so bail out | |||
| return self._reentrant_call_error() | |||
There was a problem hiding this comment.
I would prefer:
Suggested change
| return self._reentrant_call_error() | |
| raise self._reentrant_call_error() |
or:
Suggested change
| return self._reentrant_call_error() | |
| self._reentrant_call_error() |
Since the return here is a lie: the function never returns.
I'm not ok. Not only, the reproducer script fails when run with I would prefer to have a queue of pending messages, and send them once we are back to a known state: running and working resource tracker with a pipe connected to this process. I started a draft patch, but my head is blocked by the concept of reentrant call. I'm now used to multithreading where adding locks make the code correct by making sure that the code is executed in a specific order. But here a Your RLock change sounds more correct than my local change. |
|
For me, this PR changes too many things are once, I don't feel able to review it. As I wrote, I would prefer to have the RLock change done in a separated PR. So this change would smaller and it would be easier for me to think about it (and consider other implementations). |
The warning seems useful. Complaining that it fails when Would you prefer the problem to be silenced?
Sure, everyone would like something more reliable. Where is the patch?
The RLock change is trivial, so I'm not sure this would solve anything. But, mostly, the RLock change is only useful for this PR. That said, I can submit a separate PR for RLock if you're really keen on it. |
|
cc @gpshead |
|
Okay, I think I'm gonna merge this now and submit the backports. |
pitrou
added
needs backport to 3.11
|
Thanks @pitrou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
|
Thanks @pitrou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Sep 26, 2023
…cker (pythonGH-109629) --------- (cherry picked from commit 0eb9883) Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Sep 26, 2023
…cker (pythonGH-109629) --------- (cherry picked from commit 0eb9883) Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
|
GH-109897 is a backport of this pull request to the 3.11 branch. |
|
GH-109898 is a backport of this pull request to the 3.12 branch. |
|
|
|
ResourceWarnings is a call for action: you should close a resource explicitly. But here, as an user, what can you do about this messy reentrant function call issue? Well, I suppose that a log / error is better than to hang. This change may or may not fix issue gh-109593: the test may continue to fail, but in different ways. |
|
csm10495
pushed a commit
to csm10495/cpython
that referenced
this pull request
Sep 28, 2023
…cker (python#109629) --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Glyphack
pushed a commit
to Glyphack/cpython
that referenced
this pull request
Sep 2, 2024
…cker (python#109629) --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.