Repository navigation
gh-161143: Release completed buffered Pool iterator registrations - #161144
JoaoMarcos44 wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Thank you for your interest in contributing to Python. This is a community-driven project, and we want it to be a welcoming place to learn and collaborate. Contributors are responsible for reviewing and understanding what they submit, checking its claims, and being able to explain changes in their own words. This helps reviewers focus on the contribution and leaves more time to support other contributors. This contribution appears to have been authored with AI assistance. Please explain your contribution in your own words and describe how you checked its correctness. Please read our guidelines for using AI tools. |
|
In response to the request for an explanation, here is how this was found and checked. How the bug was found The investigation started with an AI-assisted source-code review of the lifetime of buffered The pool's semaphore registry is separate from the result cache. Clearing the iterator therefore leaves the semaphore reachable through the live pool, even after the caller has consumed all results. How the behavior was checked A bounded reproduction repeatedly completed ten buffered mappings through the same one-worker The added regression test, Why this change addresses it The patch removes the registry entry when completion is established, before clearing the pool reference. At that point the iterable's length is known and all results have arrived, so that iterator's task generator no longer needs a registry entry to be woken during shutdown. This does not cancel active work or discard buffered results. Termination also uses a list snapshot of the registry, matching Human validation and AI assistance The submitter has confirmed that the contribution received human validation. That confirmation is separate from the automated test executions described above; it is not being presented as a claim that a human manually reran those tests. AI assisted the investigation, implementation, test harnesses, and preparation of this explanation. The reported local runs used Windows CPython 3.15.0b2 loading the audited 3.16 module, rather than a native 3.16 build. No free-threaded runtime was tested locally. Those limits are recorded in the PR and validation notes. |
|
When convenient, @gpshead, could you please review the explanation above and take a look at the PR? Please let me know if any further information is needed to address AI-needs-explanation. Thank you. |
Closes #161143.
Fully consumed buffered
Pool.imap()andimap_unordered()iterationscurrently leave semaphore registrations behind in a live pool. This change
removes each registration at completion before clearing the iterator's
reference to the pool.
Root cause
A buffered iterator creates a
threading.Semaphoreand adds it toPool._taskqueue_buffersize_semaphores. This registry has a legitimateshutdown purpose:
close()and termination use it to wake task submissionthat may be blocked by the buffer limit.
The lifetime of that registration is longer than intended:
IMapIterator.__init__()registers the semaphore and retains the pool inself._pool.IMapIterator._set()orIMapUnorderedIterator._set()removes the iterator from the result cacheand sets
self._pool = None. The equivalent completion branch in_set_length()handles cases where the length becomes known after theresults, including empty input.
_stop_iterator()tries to discardits semaphore from the pool's registry, but only if
self._pool is not None.That reference has already been cleared, so the discard is skipped.
reference to the semaphore. Garbage collection therefore cannot reclaim
that object merely because the caller has finished consuming the results.
Removing the iterator from the result cache does not remove this separate
registration. The original implementation is in
Lib/multiprocessing/pool.pyat the audited base.Consequences and observed impact
buffered iterator, rather than per individual result. This can happen even
when the input is empty.
worker that repeatedly maps small inputs through one long-lived pool can
keep accumulating these Python objects after the real work has finished.
Additional traversal work follows from the code path; its latency impact
has not been benchmarked.
unmodified module and 0 with the patch. In a separate unpatched
2,000-iteration experiment, 2,000 semaphores remained and
tracemallocmeasured 2,693,553 additional bytes of Python allocations
(approximately 2.7 MB). This is a local measurement, not process RSS or a
production growth-rate forecast.
The evidence concerns retained Python semaphore objects. It does not show
leaked worker processes, incorrect returned results, an out-of-memory failure,
or a universal shutdown hang. No official P0/P1 severity or security impact is
claimed.
Minimal reproduction
ThreadPoolshares the affected iterator implementation with the processpool and keeps this example small:
Observed before the fix:
10. Expected after these completed iterations:0. The private registry is inspected only to expose the retainedreferences.
The same behavior was reproduced with a spawn process pool,
imap_unordered(), and chunksizes 1 and 2. A control withoutbuffersizeretained no registrations. The API being tested is from the 3.16 source;
running this snippet on an older, unmodified module that lacks
buffersizewill not reproduce this defect.
Correction and why it addresses the cause
The fix removes the semaphore from the pool's registry before clearing
self._pool. A shared_release_pool()helper is called by each completionpath and by
_stop_iterator(); usingdiscard()makes repeated removalharmless.
Completion means that the task iterable's length is known and all its results
have arrived. At that point, that iterator's task generator no longer needs
the registry entry to be woken during shutdown. This does not cancel pending
work, consume the caller's results, or destroy the pool. The iterator still
owns its own semaphore reference while it needs it.
The completion and exhaustion paths use the iterator's existing condition
lock. Since the result handler can now remove registry entries, termination
uses a
list(set)snapshot, matching the existingclose()implementation.The snapshot rationale was reviewed against the CPython implementation and a
controlled iterator interleaving; it was not validated on a free-threaded
runtime.
Changes and regression coverage
Lib/multiprocessing/pool.py: release registrations from ordered,unordered and length-setting completion paths, and take a list snapshot
when termination processes the registry.
Lib/test/_test_multiprocessing.py: addtest_imap_with_buffersize_releases_semaphores, covering ordered andunordered iteration, chunksizes 1 and 2, and empty/nonempty inputs.
It checks both returned results and the absence of stale registrations.
Manager pools are skipped because the check requires the real pool's
internal registry.
Misc/NEWS.d/next/Library/: add a Library entry describing the memory-leakfix.
The test checks the registry after iterator exhaustion and the existing
condition-lock synchronization, without relying on an arbitrary sleep.
The patch does not change the public method signatures or buffer-size policy.
Code and test commit.
git diff --checkpassed for the submitted changes.Validation environment and limits
Local validation used Windows CPython 3.15.0b2 loading the audited 3.16
multiprocessing.poolsource, based on4b21d154b4864531641f90e1b87108575e673d75.It was not a native CPython 3.16 build or its native regrtest runner.
No free-threaded runtime was executed locally.
The new regression method produced 16 failing subtests across the thread and
process runs before the fix and none afterward. A selection of 64 test cases
and 68 additional bounded scenarios passed with the patched module. The
selected-method harness extracted source test methods and supplied their
parameters for the 3.15 host; these counts must not be read as a full native
3.16 test-suite run. The additional scenarios cover worker/iterable errors,
out-of-order results, and concurrent completion/shutdown.
Detailed validation and suggested native commands
provide the scope and limitations of these checks.
Victorian infographic
The ledger represents the pool's registry and the tokens represent Python
semaphore objects. The illustration shows why completed work must release
its registry entry; it does not depict leaked OS handles or imply that active
work is cancelled.
AI tools assisted the implementation, test harnesses, review and illustration.