Skip to content

Fix memory leak: track live children with one shutdown handler - #15

Open
flavio-fernandes wants to merge 1 commit into
mortoray:masterfrom
flavio-fernandes:fix/atexit-handler-leak
Open

Fix memory leak: track live children with one shutdown handler#15
flavio-fernandes wants to merge 1 commit into
mortoray:masterfrom
flavio-fernandes:fix/atexit-handler-leak

Conversation

@flavio-fernandes

@flavio-fernandes flavio-fernandes commented Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Fixes unbounded handle retention in long-running Group users. The previous implementation registered one atexit closure per child, keeping completed Popen objects reachable for the lifetime of the interpreter.

This change:

  • replaces per-child callbacks with one module-level shutdown handler;
  • tracks only live child handles and removes them through exception-safe cleanup after actual process exit;
  • flattens reader control flow and guarantees the completion sentinel even when a caller-supplied error callback raises;
  • resets inherited handles and locks after POSIX forks while preserving imports on non-forking platforms;
  • retains a PID ownership guard as shutdown protection; and
  • requires Python 3.7 or newer.

Regression coverage includes collectability, atexit registry churn, stream and callback failures, children outliving output, inherited handle isolation, child lock reset and re-arming, and import without register_at_fork.

Verification: 25 tests passed on CPython 3.11.16 and 3.14.7.

Closes #14

@flavio-fernandes flavio-fernandes mentioned this pull request Sep 8, 2026
Open
@flavio-fernandes

flavio-fernandes commented Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

@mortoray — this is ready for review. The final version is one amended, squashed commit (ff94d43) with all 25 tests passing on CPython 3.11.16 and 3.14.7 and no changes to the existing Group API. The reader block is flattened and now guarantees its completion sentinel even when on_error raises; the PR description contains the condensed problem, implementation, portability, and verification summary.

@flavio-fernandes
flavio-fernandes marked this pull request as draft September 8, 2026 18:22
@flavio-fernandes
flavio-fernandes marked this pull request as ready for review September 9, 2026 01:38
flavio-fernandes added a commit to flavio-fernandes/mqtt2cmd that referenced this pull request Sep 9, 2026
@flavio-fernandes @claude
Fix subprocess atexit handler memory leak (#3)
4a32f73 
Group._run_impl registered an atexit handler per spawned process and never
released it. The handler closed over the Popen handle, so the registry kept
a strong reference to every process a group had ever run, uncollectable even
after the child exited. On this Pi Zero W, dispatching ~18,000 commands a
day, the mqtt2cmd dispatcher grew ~655 bytes per command -- about 11 MB/day,
exhausting a 437 MB box every couple of weeks.

Replace the per-child closures with one module-level handler that tracks
only live handles. Unregistering per child is not sufficient: before CPython
3.14 atexit.unregister frees the callback but never reclaims its registry
slot, so the array still grows by one pointer per command.

Drop a handle only once the child has actually exited, not when its output
reaches EOF -- a command that closes or replaces both stdout and stderr
while still running hits EOF immediately, and releasing there would orphan
it at interpreter exit.

Make the reader cleanup exception-safe. stdin.close() can raise
BrokenPipeError; escaping there skipped both the decrement of self.waiting,
leaving readlines() waiting forever, and the discard, stranding the handle
for the interpreter's life. One outer try/finally now covers closing, the
decrement, the wait and the discard.

Reset inherited state in POSIX fork children. Waiting for real exit means a
reader thread holds each handle's Popen._waitpid_lock for the child's life,
so a forked child inherits it locked, poll() cannot read the real status,
and terminate() would signal a process the child does not own. A PID guard
makes the handler inert in any process that did not create the registry, and
an at-fork hook re-arms tracking where the API exists.

This mirrors the upstream fix in mortoray/shelljob#15, from which proc.py is
vendored; the shared code is line-for-line identical apart from indentation.

Add regression coverage for collection, registry churn, stream errors,
output-closing children, fork ownership and re-arming, and platforms without
register_at_fork.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@flavio-fernandes
Fix Group shutdown handle leak
ff94d43 
Replace per-child atexit closures with one module-level handler that tracks only live Popen handles. Make reader cleanup exception-safe, always emit the completion sentinel, and retain handles until the child actually exits.

Reset inherited handles and locks in POSIX fork children, retain the PID safety guard, and keep imports portable on non-forking platforms. Require Python 3.7 or newer.

Add regression coverage for collection, registry churn, stream and callback errors, output-closing children, fork ownership and re-arming, and platforms without register_at_fork.

Closes mortoray#14
flavio-fernandes added a commit to flavio-fernandes/mqtt2cmd that referenced this pull request Sep 9, 2026
@flavio-fernandes @claude
Fix subprocess atexit handler memory leak (#3)
cef16e3 
Group._run_impl registered an atexit handler per spawned process and never
released it. The handler closed over the Popen handle, so the registry kept
a strong reference to every process a group had ever run, uncollectable even
after the child exited. On this Pi Zero W, dispatching ~18,000 commands a
day, the mqtt2cmd dispatcher grew ~655 bytes per command -- about 11 MB/day,
exhausting a 437 MB box every couple of weeks.

Replace the per-child closures with one module-level handler that tracks
only live handles. Unregistering per child is not sufficient: before CPython
3.14 atexit.unregister frees the callback but never reclaims its registry
slot, so the array still grows by one pointer per command.

Drop a handle only once the child has actually exited, not when its output
reaches EOF -- a command that closes or replaces both stdout and stderr
while still running hits EOF immediately, and releasing there would orphan
it at interpreter exit.

Make the reader cleanup exception-safe. stdin.close() can raise
BrokenPipeError; escaping there skipped both the decrement of self.waiting,
leaving readlines() waiting forever, and the discard, stranding the handle
for the interpreter's life. A single try/except/finally now covers the
completion sentinel, closing, the decrement, the wait and the discard, so no
path can skip them.

Reset inherited state in POSIX fork children. Waiting for real exit means a
reader thread holds each handle's Popen._waitpid_lock for the child's life,
so a forked child inherits it locked, poll() cannot read the real status,
and terminate() would signal a process the child does not own. A PID guard
makes the handler inert in any process that did not create the registry, and
an at-fork hook re-arms tracking where the API exists.

This mirrors the upstream fix in mortoray/shelljob#15, from which proc.py is
vendored. The shared code is line-for-line identical apart from indentation;
the only divergences are upstream's encoding-aware readline sentinel and its
on_error callback, neither of which exists here.

Add regression coverage for collection, registry churn, stream errors,
output-closing children, fork ownership and re-arming, and platforms without
register_at_fork.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@flavio-fernandes

flavio-fernandes commented Sep 10, 2026

Copy link
Copy Markdown
Author

/assign @mortoray

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Memory leak: Group.run() registers an atexit handler per process that is never unregistered

1 participant

@flavio-fernandes