Skip to content

fix(foundation): leaving an Extractor's with-block closes a stream the caller owns #1999

Closed

Description

@JarryShaw

with Extractor(fin=stream) as e: closes stream on exit, even when the caller opened it and still owns it. This breaks the ownership rule from #610: a stream the caller supplied is the caller's to close. _cleanup and the abandoned-extraction backstop both follow that rule. Extractor.__exit__ calls self._ifile.close() unconditionally.

  • Repro (main): s = open('examples/captures/arp.pcap', 'rb'), then with pcapkit.extract(fin=s, nofile=True, auto=True): pass. After that, s.closed is True. Without the with, it stays False.
  • Fix: close the input in __exit__ only when _owns_input() holds, as _cleanup does. Add a test for a caller's seekable stream and for a non-seekable one wrapped in SeekableReader.

Found while working on #1997, in #1998.

Activity

  1. added
    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)
    fixPull requests that fix a defect (fix: subject prefix)
    pendingAccepted and unstarted - nobody is on it and nothing blocks it
    on Oct 20, 2026
  2. added
    wipWork in flight - a covering PR is open or an agent is actively on it
    and removed
    pendingAccepted and unstarted - nobody is on it and nothing blocks it
    on Oct 20, 2026
  3. JarryShaw commented on Oct 20, 2026

    @JarryShaw
    OwnerAuthor

    This needs your call: it reverses a choice made in #636.

    #636 kept __exit__ closing unconditionally, on the reasoning that a with block asks for the close. Changing it is a contract change, not a defect fix. That reasoning came from the PR's author, not from a ruling of yours. The 1.5.0 changelog says the same, and test_extraction.py pins it.

    Option 1 (recommended): __exit__ follows the ownership rule. It closes only what pcapkit opened, like _cleanup and the GC backstops. A caller's stream stays open after the with block, and a path is still closed. The fix is ready on fix/1999-exit-ownership (883c87960, stacked on #1998). It updates the changelog line and the pinned test, and adds tests that fail without it.

    Option 2: keep the current behaviour. Close this issue, and the __exit__ docstring states the exception to #610's rule.

  4. added
    needs: decisionWaiting on the maintainer to decide — not blocked by other work
    and removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Oct 20, 2026
  5. moved this to Needs decision in PyPCAPKiton Oct 20, 2026
  6. JarryShaw commented on Oct 20, 2026

    @JarryShaw
    OwnerAuthor

    take option 1

  7. JarryShaw commented on Oct 20, 2026

    @JarryShaw
    OwnerAuthor

    Going with option 1, as ruled: #2000 makes __exit__ follow the ownership rule. It is stacked on #1998, so merge that first. Labelled breaking, because a caller's stream now stays open after the with block. A cross-review is running.

  8. added
    wipWork in flight - a covering PR is open or an agent is actively on it
    and removed
    needs: decisionWaiting on the maintainer to decide — not blocked by other work
    on Oct 20, 2026
  9. moved this from Needs decision to WIP in PyPCAPKiton Oct 20, 2026
  10. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Oct 21, 2026
  11. added this to the PyPCAPKit v1.5 milestone on Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)fixPull requests that fix a defect (fix: subject prefix)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions