Skip to content

fix(foundation): back the abandoned-extraction close with weakref.finalize (#1997) - #1998

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1997-extractor-finalize
Oct 20, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1997-extractor-finalize

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 20, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

On GraalPy, an extraction abandoned before EOF never closed the file it had opened, because GraalPy never calls Extractor.__del__.

The fix. Extractor.__init__ now also registers a weakref.finalize, stored as _finalizer. It works as follows:

  • It closes over the stream alone, so it keeps no reference to the extractor.
  • It is registered only when _owns_input() holds, so a stream the caller passed in is never closed.
  • _cleanup and __exit__ detach it, so a finished extraction is not closed twice.

__del__ stays, because tests and subclasses call it directly. Both backstops share _release_input.

Tests. The new FinaliserBackstopWithoutDel tests in test_extraction_ownership.py fail without the fix on CPython (6 failed) and on GraalPy 25.4.4 (8 failed). They pass on both with it. The other tests/foundation extraction files pass on both interpreters.

Found separately: __exit__ closes a caller-supplied stream too, which this PR leaves as it is (#1999).

Closes #1997

…alize (#1997)

GraalPy never calls `__del__`, so an `auto=False` extraction dropped before
EOF kept the file it opened. `Extractor.__init__` now registers a
`weakref.finalize` over the input stream alone, and only for a stream it
owns, so a caller's stream is never closed by it. `_cleanup` and `__exit__`
detach it, and `__del__` stays, sharing the same guarded close.

New tests check the finaliser on CPython too: it is registered over the
stream rather than the extractor, closes an owned handle with `__del__`
patched out, is detached on a finished extraction, and spares a caller's
stream, plain or wrapped.
@JarryShaw JarryShaw added the fix Pull requests that fix a defect (fix: subject prefix) label Oct 20, 2026
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 20, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 33dddb93b. A Sonnet review checked the fix on CPython 3.14 and GraalPy 25.4.4.

  • No keep-alive. The finalizer's only argument is the stream (peek() gives args == (_ifile,)). An extractor that outlives del is held by the existing engine cycle, not by the finalizer: it stays alive with the finalizer detached too.
  • Ownership. A caller's seekable stream gets no finalizer. A caller's non-seekable stream stays open after the SeekableReader wrapper is collected, because stream_closing is off.
  • No accumulation. 200 finished extractions left nothing in weakref.finalize._registry. Abandoned ones drain on gc.collect().
  • Tests. Reverting extraction.py fails 6 on CPython and 8 on GraalPy, including the existing abandoned-handle test. Everything passes with the fix.

One behaviour change, noted here: atexit is on, so a live extractor's own file is now closed at interpreter exit rather than left to a ResourceWarning.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 20, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: 91.52% (unit tier, Python 3.15, 33dddb93b, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 19597 881 2618 867 92.11%
pcapkit/corekit 2540 71 808 37 96.54%
pcapkit/dumpkit 276 3 100 3 98.40%
pcapkit/foundation 2929 23 1020 25 98.78%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17450 226 4598 191 98.09%
pcapkit/toolkit 872 36 298 10 95.21%
pcapkit/utilities 522 2 158 4 99.12%
pcapkit/vendor 4431 1995 1018 167 51.73%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 390cde9 into main Oct 20, 2026
60 checks passed
@JarryShaw
JarryShaw deleted the fix/1997-extractor-finalize branch October 20, 2026 22:01
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 20, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 20, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(foundation): an abandoned extraction keeps its file open on GraalPy, which never calls __del__

1 participant