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
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