From daa76d691486ab85f88f58e94b9e47d84d6fbd0f Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 20 Oct 2026 14:56:14 -0400 Subject: [PATCH] fix(foundation): close only an owned input on leaving the with block (#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 --- CHANGELOG.md | 2 +- docs/source/changelog/1.5.0.rst | 5 +- pcapkit/foundation/extraction.py | 25 +++++- tests/_support.py | 5 +- tests/foundation/test_extraction.py | 10 +++ tests/foundation/test_extraction_ownership.py | 76 +++++++++++++++++++ 6 files changed, 113 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e5fced5b38..141ae0626f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -139,7 +139,7 @@ Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` to `1.5.0b3` (all 2026-09-19) and - TCP reassembly mixed absolute sequence numbers with buffer-relative slicing, so on any capture carrying a SYN with a realistic initial sequence number incomplete datagrams were dropped silently, and the `completed=False` branch of the public API was unreachable ([#349](https://github.com/JarryShaw/PyPCAPKit/issues/349), [#376](https://github.com/JarryShaw/PyPCAPKit/pull/376)). - `format='text'` raised `AttributeError` before writing anything, naming a `dictdumper.Text` that has never existed. It now points at `Tree`, as the `'txt'` alias beside it already did. -- `Extractor` had the ownership of its input stream inverted: a handle it opened itself, from `fin` given as a path, was **never closed**, while a stream the *caller* supplied and still needed **was**. `_cleanup` closed under `not self._flag_s` (the flag that gated the `open()`), and the `SeekableReader` wrapping a non-seekable input asked for `stream_closing=not self._flag_s` in the same wrong direction. The leak was one descriptor per extraction, the root cause of the `ResourceWarning` that reddened [#577](https://github.com/JarryShaw/PyPCAPKit/pull/577), [#596](https://github.com/JarryShaw/PyPCAPKit/pull/596) and [#600](https://github.com/JarryShaw/PyPCAPKit/pull/600) ([#606](https://github.com/JarryShaw/PyPCAPKit/issues/606) had only made the test immune). Both conditions now follow ownership via a single `Extractor._owns_input`, and a finaliser releases the handle of an extraction *abandoned* before end of file (`auto=False`, iterated part way and dropped); the files [#610](https://github.com/JarryShaw/PyPCAPKit/issues/610) names emit no such warning. `__exit__` deliberately still closes unconditionally, since a caller who scopes an `Extractor` with `with` asked for that ([#610](https://github.com/JarryShaw/PyPCAPKit/issues/610)). +- `Extractor` had the ownership of its input stream inverted: a handle it opened itself, from `fin` given as a path, was **never closed**, while a stream the *caller* supplied and still needed **was**. `_cleanup` closed under `not self._flag_s` (the flag that gated the `open()`), and the `SeekableReader` wrapping a non-seekable input asked for `stream_closing=not self._flag_s` in the same wrong direction. The leak was one descriptor per extraction, the root cause of the `ResourceWarning` that reddened [#577](https://github.com/JarryShaw/PyPCAPKit/pull/577), [#596](https://github.com/JarryShaw/PyPCAPKit/pull/596) and [#600](https://github.com/JarryShaw/PyPCAPKit/pull/600) ([#606](https://github.com/JarryShaw/PyPCAPKit/issues/606) had only made the test immune). Both conditions now follow ownership via a single `Extractor._owns_input`, and a finaliser releases the handle of an extraction *abandoned* before end of file (`auto=False`, iterated part way and dropped); the files [#610](https://github.com/JarryShaw/PyPCAPKit/issues/610) names emit no such warning. Leaving a `with` block follows the same rule, so a caller's stream stays open after it ([#1999](https://github.com/JarryShaw/PyPCAPKit/issues/1999)). - `extract(..., no_eof=True)` never returned. End of stream was detected and `ExtractionWarning: EOF reached` fired, but every loop that handles it (`record_frames`, `__next__`, `__call__`) read `if self._flag_n: continue`, so the flag suppressed the error that *ended* the loop without supplying another ending and an exhausted input was retried forever. The flag is not pointless -- `__main__` sets `no_eof` for `fin='-'`, and the end of what has arrived on a live capture is not the end of the capture -- so this is a termination condition rather than a deletion. The input's own position tells a paused capture from a finished one: `prepare` raises end of stream when zero bytes remain and restores the position first, so two consecutive ends of stream at the same position mean nothing arrived between them (`Extractor._note_eof_progress`). A pipe is unaffected, since a read there *blocks* while the writer is open but idle. This also fixes `pcapkit -`, which hung on **any** finished stdin. **One deliberate narrowing**: a *seekable* input does not block, so a file still being appended to ends at the data present when the extraction reached it; a timed grace period would make the cut-off intermittent rather than absent, so following a growing file is left to a policy of its own. Pre-existing and untouched: an append landing *mid-record* raises `ValueError: read length must be non-negative or -1`. The end-to-end tests are bounded by a **child process** rather than `tests._support.time_limit`, because the in-process deadline proved intermittent on this loop, and an intermittent guard against a hang leaves a wedged suite rather than a red test. Guesses at the cause are ruled out: the parse path's `except Exception` handlers are never entered during the spin, and the memory is the retry loop's `EOF reached` records, which the runner retains ([#620](https://github.com/JarryShaw/PyPCAPKit/issues/620)). - `register_protocol` displaced one protocol class with another and said nothing. The registry behind `pcapkit.protocols.__proto__` is keyed on `cls.__name__.upper()`, and several dispatchable classes are named `HTTP` (the base `pcapkit.protocols.application.http.HTTP` and the `httpv1` and `httpv2` implementations), so a bare assignment decided the winner: `register_protocol(httpv2.HTTP)` moved `__proto__['HTTP']` to the `httpv2` class with no exception, log line or `RegistryWarning`, and `ProtocolBase.expand_comp('HTTP')` then resolved to whichever registered last. The overwrite now raises a `RegistryWarning` naming the displaced and the replacing class. The sharpest path is not a call to the registrar: `Protocol.__init_subclass__()` registers unconditionally, so merely *defining* a subclass of the public `Protocol` under a name a built-in holds displaced that built-in. A plain `import pcapkit` emits zero new warnings: the built-in `HTTP` classes derive from `ProtocolBase`, so `__init_subclass__` never fires for them, and import-time seeding keys off the distinct names `HTTP`, `HTTPv1` and `HTTPv2`. The guard deliberately reads "key present and the incumbent is a different class", not mere presence ([#726](https://github.com/JarryShaw/PyPCAPKit/pull/726) later gave its sibling registrars the same identity guard): this function is the funnel every wrapper registrar ends in, so registering one class under two codes (`register_tcp` then `register_udp`, a documented path) reaches it twice with nothing displaced. The collision is reported, not resolved: re-keying is a registry-format change that none of the bare-name readers can absorb (each degrades *silently* to a string or `Raw` on a miss), so it belongs to the registry redesign in [#514](https://github.com/JarryShaw/PyPCAPKit/issues/514). The issue called this the one registrar with no warning; in fact none warned by itself, the siblings only by delegating to a guarded classmethod ([#675](https://github.com/JarryShaw/PyPCAPKit/issues/675)). - the `#:` autodoc comments in `pcapkit/foundation/traceflow/traceflow.py` that named a bare `Type`, which Sphinx resolves against every class named `Type` in the project rather than `typing.Type`, and silently linked to `pcapkit.const.l2tp.type.Type`, an L2TP field-type enum; once [#684](https://github.com/JarryShaw/PyPCAPKit/issues/684) rendered `TraceFlow._foutio`, the built docs pointed at the wrong class with no warning. These sites now spell it `~typing.Type[Dumper]`, among the sites [#709](https://github.com/JarryShaw/PyPCAPKit/issues/709) lists. The other bare `Type` sites, in hand-written `:type:` fields under `docs/source/pcapkit/foundation/`, are fixed separately by [#714](https://github.com/JarryShaw/PyPCAPKit/pull/714) ([#709](https://github.com/JarryShaw/PyPCAPKit/issues/709)). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 0d9dbead5e..5dbf472157 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -976,9 +976,8 @@ Fixed Both conditions now follow ownership via a single ``Extractor._owns_input``, and a finaliser releases the handle of an extraction *abandoned* before end of file (``auto=False``, iterated part way and dropped); the files :issue:`610` names - emit no such warning. ``__exit__`` deliberately still - closes unconditionally, since a caller who scopes an :class:`.Extractor` with ``with`` - asked for that (:issue:`610`). + emit no such warning. Leaving a ``with`` block follows the same rule, so a caller's + stream stays open after it (:issue:`1999`). * ``extract(..., no_eof=True)`` never returned. End of stream was detected and ``ExtractionWarning: EOF reached`` fired, but every loop that handles it (``record_frames``, ``__next__``, ``__call__``) read ``if self._flag_n: continue``, diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index 286222533e..8b18b5b182 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -1590,9 +1590,26 @@ def __enter__(self) -> 'Extractor': def __exit__(self, exc_type: 'Type[BaseException] | None', exc_value: 'BaseException | None', traceback: 'TracebackType | None') -> 'None': # pylint: disable=unused-argument - """Close the input file when exits.""" + """Release what this class holds when the ``with`` block exits. + + The engine is always closed. The input stream is closed only *if this + class owns it* -- the rule of :issue:`610`, as :meth:`_owns_input` writes + it down and :meth:`_cleanup` and the backstops already follow. A stream + the caller supplied stays open for the caller to close, so leaving the + block no longer closes it behind the caller's back (:issue:`1999`). + + For a caller's non-seekable stream, the stream this class owns is the + :class:`~pcapkit.corekit.io.SeekableReader` wrapped around it, and that + wrapper is closed; it was built with ``stream_closing`` off, so closing + it leaves the caller's stream underneath open. + + """ logger.debug('closing %s on context exit after %d frame(s)', self._ifnm, self._frnum) - self._ifile.close() + # NOTE: ownership, not the ``with`` statement, decides who closes the + # input (#610, #1999): entering the block does not hand the caller's + # stream over to this class. + if self._owns_input(): + self._ifile.close() self._detach_finalizer() self._exeng.close() @@ -1643,8 +1660,8 @@ def _owns_input(self) -> 'bool': """Whether the input stream is this class's to close. The one place the ownership rule of :issue:`610` is written down, so that - :meth:`_cleanup`, :meth:`__del__` and the registration of - :attr:`_finalizer` cannot drift apart on it. + :meth:`_cleanup`, :meth:`__exit__`, :meth:`__del__` and the registration + of :attr:`_finalizer` cannot drift apart on it. Returns: :data:`True` when the input stream was opened by this class and is diff --git a/tests/_support.py b/tests/_support.py index 0edc4a27cc..fb1e48f8dc 100644 --- a/tests/_support.py +++ b/tests/_support.py @@ -1135,8 +1135,9 @@ def close_extractor(extractor: object) -> None: :meth:`Extractor._cleanup ` or :meth:`Extractor.__exit__ `, so nothing in the library closes up after them. Both of those close the - input file *and* the engine, and this helper has to do the same: the input - file is not the only resource. The ``pcap_ct`` and ``pypcap`` engines hold a + engine as well as an input file the extractor opened itself, and this helper + closes both, whoever opened the input: the input file is not the only + resource. The ``pcap_ct`` and ``pypcap`` engines hold a live :class:`pcap.pcap` handle, and ``pyshark`` holds a temporary file, so dropping the extractor without closing the engine leaks an OS-level handle per test. Under a suite that builds hundreds of extractors that accumulates diff --git a/tests/foundation/test_extraction.py b/tests/foundation/test_extraction.py index 7da9238b73..0d7670574c 100644 --- a/tests/foundation/test_extraction.py +++ b/tests/foundation/test_extraction.py @@ -949,11 +949,21 @@ def test_record_header_record_frames_iteration_call_and_cleanup(self) -> None: given_stream._cleanup() self.assertFalse(given_stream._ifile.closed_by_test) + # Leaving the ``with`` block follows the same ownership rule: it closes + # a handle ``Extractor`` opened, and leaves a caller's stream open. This + # assertion closed the caller's stream until #1999. + manual._flag_s = True with manual as ctx: self.assertIs(ctx, manual) self.assertTrue(manual._ifile.closed_by_test) self.assertGreaterEqual(manual._exeng.close_calls, 1) + scoped_stream = self._bare_extractor() + with scoped_stream: + pass + self.assertFalse(scoped_stream._ifile.closed_by_test) + self.assertGreaterEqual(scoped_stream._exeng.close_calls, 1) + def test_constructor_initialises_without_running_when_patched(self) -> None: from pcapkit.foundation.extraction import Extractor diff --git a/tests/foundation/test_extraction_ownership.py b/tests/foundation/test_extraction_ownership.py index 3c965bc88e..e68d31f813 100644 --- a/tests/foundation/test_extraction_ownership.py +++ b/tests/foundation/test_extraction_ownership.py @@ -337,6 +337,82 @@ def test_non_seekable_stream_survives_its_seekable_wrapper(self) -> None: stream.close() +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class ContextExitFollowsOwnership(unittest.TestCase): + """#1999: leaving a ``with`` block closes only what ``Extractor`` owns. + + ``__exit__`` closed the input unconditionally, so scoping an extraction with + ``with`` closed a stream the caller had passed in and still held, which the + rule of #610 says is the caller's to close. + + """ + + def test_callers_seekable_stream_stays_open(self) -> None: + """The issue's own repro: an ``auto=True`` extraction inside ``with``.""" + import pcapkit + + with open(sample_path('in.pcap'), 'rb') as handle: + with pcapkit.extract(fin=handle, nofile=True, store=False): + pass + + with self.subTest(check='handle not closed'): + self.assertFalse(handle.closed, + "leaving the with block closed the caller's stream") + + with self.subTest(check='handle still readable'): + handle.seek(0) + self.assertEqual(handle.read(4), b'\xd4\xc3\xb2\xa1') + + def test_callers_non_seekable_stream_stays_open(self) -> None: + """The wrapper ``Extractor`` built is closed; the stream under it is not.""" + import pcapkit + from pcapkit.corekit.io import SeekableReader + + with open(sample_path('in.pcap'), 'rb') as source: + payload = source.read() + + # The ``sys.stdin.buffer`` shape: see + # ``test_non_seekable_stream_survives_its_seekable_wrapper`` for why a + # bare non-seekable ``BytesIO`` will not do. + stream = io.BufferedReader(_NonSeekableRaw(payload)) + self.addCleanup(stream.close) + with pcapkit.extract(fin=stream, nofile=True, store=False, + auto=False) as extractor: + next(extractor) + + self.assertIsInstance(extractor._ifile, SeekableReader, + 'a non-seekable input should have been wrapped, ' + 'without which this test proves nothing') + with self.subTest(check='wrapper closed'): + self.assertTrue(extractor._ifile.closed, + 'leaving the with block should close the wrapper ' + 'Extractor built') + with self.subTest(check="caller's stream still open"): + self.assertFalse(stream.closed, + "leaving the with block closed the caller's stream " + 'through the wrapper') + + def test_path_opened_handle_is_closed(self) -> None: + """A handle ``Extractor`` opened from a path is closed on leaving the block. + + ``auto=False`` and one frame read, so the extraction is still short of end + of file and ``_cleanup`` has not run: the close is ``__exit__``'s alone. + + """ + import pcapkit + + with pcapkit.extract(fin=sample_path('in.pcap'), nofile=True, store=False, + auto=False) as extractor: + next(extractor) + self.assertFalse(extractor._flag_e, + '_cleanup ran inside the block, so this test no ' + 'longer measures __exit__') + self.assertFalse(extractor._ifile.closed) + + self.assertTrue(extractor._ifile.closed, + 'leaving the with block left the handle Extractor opened') + + @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class FinaliserBackstopWithoutDel(unittest.TestCase): """#1997: the backstop is a :class:`weakref.finalize`, not ``__del__`` alone.