Skip to content

fix(foundation): close only an owned input on leaving the with block (#1999) - #2000

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1999-exit-ownership
Oct 21, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1999-exit-ownership

Conversation

@JarryShaw

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

Stacked on #1998. This PR carries #1998's commit, so merge #1998 first. After that, the diff is this PR's commit alone.

Option 1 from #1999. Leaving an Extractor's with block now closes the input only if _owns_input() holds. That is the #610 rule, and _cleanup and the backstops already follow it. So:

  • A stream the caller passed in stays open, whether seekable or wrapped in SeekableReader.
  • A path pcapkit opened is still closed.

This reverses the choice #636 made. The 1.5.0 changelog line and the test_extraction.py case that pinned the old close are updated to match.

New ContextExitFollowsOwnership tests in test_extraction_ownership.py cover:

  • a caller's seekable stream;
  • a caller's non-seekable stream;
  • a path closed by __exit__ alone.

With the fix reverted, the seekable case fails (2 SUBFAILED), as does test_extraction.py (1 failed).

Closes #1999

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) breaking Breaks public-facing behaviour or API (apply alongside the type label) labels Oct 20, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit 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 883c87960, reviewing this PR's own commit on top of #1998. A Sonnet review confirmed:

  • __exit__ closes the input only when _owns_input() holds, and still detaches the finaliser and closes the engine.
  • A path, or a SeekableReader this class built, is still closed.
  • A caller's non-seekable stream stays open, because the wrapper's stream_closing is off.
  • A second close after _cleanup is a no-op.
  • Reverting extraction.py fails the seekable case and the old pin in test_extraction.py. The non-seekable and path tests are regression guards.

No docs or example uses with on a caller's stream: examples/generators/dispatch.py passes a path.

@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

Copy link
Copy Markdown
Owner Author

f56dc5cd6 regenerates CHANGELOG.md with util/changelog_md.py. The 1.5.0 entry's edit had left it stale, which failed Changelog drift and test_committed_changelog_is_in_step_with_the_newest_entry. Nothing else changed, so the verdict stands. tests/project/test_changelog_md.py passes locally (68 passed).

@JarryShaw
JarryShaw force-pushed the fix/1999-exit-ownership branch from 883c879 to f56dc5c Compare October 20, 2026 21:16
@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 91.54% (unit tier, Python 3.15, daa76d691, 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 2930 23 1022 25 98.79%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 17468 222 4600 187 98.13%
pcapkit/toolkit 872 36 298 10 95.21%
pcapkit/utilities 523 2 158 4 99.12%
pcapkit/vendor 4431 1995 1018 167 51.73%

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

…1999)

Extractor.__exit__ closed the input unconditionally, so a with block
closed a stream the caller supplied and still owned, against the
ownership rule of #610 that _cleanup and the backstops follow. It now
closes the input only when _owns_input() holds; the engine is still
closed and the finaliser still detached. A caller's non-seekable stream
stays open, as the SeekableReader wrapper is built with stream_closing
off.

- tests: a caller's seekable and non-seekable stream stay open after the
  block, and a path-opened handle is closed by __exit__ alone
- test_extraction: the case pinning the old close now splits owned and
  caller-supplied input
- changelog and close_extractor prose no longer say the block always
  closes the input
@JarryShaw

Copy link
Copy Markdown
Owner Author

Rebased onto main as daa76d691, now that #1998 has landed as a squash: the diff is this PR's own commit only. git range-diff shows the patch identical, so the verdict stands. Extraction and changelog tests pass.

@JarryShaw
JarryShaw force-pushed the fix/1999-exit-ownership branch from f56dc5c to daa76d6 Compare October 20, 2026 22:16
@JarryShaw
JarryShaw merged commit a63cc78 into main Oct 21, 2026
56 checks passed
@JarryShaw
JarryShaw deleted the fix/1999-exit-ownership branch October 21, 2026 00:51
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 21, 2026
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 21, 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

breaking Breaks public-facing behaviour or API (apply alongside the type label) 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): leaving an Extractor's with-block closes a stream the caller owns

1 participant