From 972f5bcd266d673ed7628144273520293ae80117 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 23 Oct 2026 20:38:11 -0400 Subject: [PATCH] fix(foundation): close the engine too when the Extractor constructor raises after choosing one (#2107) - Extractor: the constructor's error handler now calls _cleanup(failed=True) instead of closing the input by hand, so the chosen engine is closed too. With failed set, the flow tracer is not finished and _flag_e is not set, a missing engine (unknown format) is skipped, and an Exception from the engine's close is logged so the original error still propagates. - Extractor: _cleanup and __exit__ now share one close path, _release. __exit__ behaves as before: no trace finish, and close errors propagate. - PyShark: close() is a no-op before run() opened the capture. - Tests: extend the #2107 unit tests to cover a failed constructor not finishing the trace, the no-engine path, and __exit__ closing through _release; select the fake engine the way _select_engine does since #2113. Tests: #2107 file 8 passed; related extraction tests 127 passed; foundation plain-unittest leg 730 tests, 0 failures, 0 errors. --- docs/source/pcapkit/foundation/extraction.rst | 1 + pcapkit/foundation/engines/pyshark.py | 7 +- pcapkit/foundation/extraction.py | 81 ++++-- ...ion_constructor_closes_engine_2107_unit.py | 261 ++++++++++++++++++ 4 files changed, 333 insertions(+), 17 deletions(-) create mode 100644 tests/foundation/test_extraction_constructor_closes_engine_2107_unit.py diff --git a/docs/source/pcapkit/foundation/extraction.rst b/docs/source/pcapkit/foundation/extraction.rst index 01eea3eb7b..8195212d63 100644 --- a/docs/source/pcapkit/foundation/extraction.rst +++ b/docs/source/pcapkit/foundation/extraction.rst @@ -58,6 +58,7 @@ and extracts parameters from a PCAP file. :no-value: .. automethod:: _cleanup + .. automethod:: _release .. autoattribute:: _flag_a .. autoattribute:: _flag_d diff --git a/pcapkit/foundation/engines/pyshark.py b/pcapkit/foundation/engines/pyshark.py index 12d4a476ad..08b4185203 100644 --- a/pcapkit/foundation/engines/pyshark.py +++ b/pcapkit/foundation/engines/pyshark.py @@ -326,5 +326,10 @@ def close(self) -> 'None': This method is to be used for closing the engine instance. It is to close the engine instance after the extraction process is finished. + A no-op if :meth:`run` never opened the capture, which is the case when + the :class:`~pcapkit.foundation.extraction.Extractor` constructor raises + before it (:issue:`2107`). + """ - self._extmp.close() + if self._extmp is not None: + self._extmp.close() diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index 769cba2fda..e1d973077d 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -1532,12 +1532,12 @@ def __init__(self, self.run() # start extraction except BaseException: # NOTE: a constructor that raises leaves the caller nothing to close, - # and Windows cannot delete a file still open (#2100). Only the input - # is released: ``_cleanup`` would need an engine, which an unknown - # format never gets, and would finish a trace that never ran. - if self._owns_input(): - self._ifile.close() - self._detach_finalizer() + # and Windows cannot delete a file still open (#2100). ``_cleanup`` + # releases the input and the engine, if one was chosen (#2107); + # ``failed`` keeps it from finishing a trace whose capture never + # reached its end, and from letting an error out of the engine's close + # replace the one being handled here. + self._cleanup(failed=True) raise def __iter__(self) -> 'Extractor': @@ -1625,15 +1625,20 @@ class owns it* -- the rule of :issue:`610`, as :meth:`_owns_input` writes wrapper is closed; it was built with ``stream_closing`` off, so closing it leaves the caller's stream underneath open. + Both go through :meth:`_release`, the close path :meth:`_cleanup` shares. + Unlike :meth:`_cleanup`, this neither finishes the flow tracer nor sets + :attr:`_flag_e`: leaving the block is not the end of the capture -- an + extraction with ``auto=False`` may be left before end of file, or on an + error -- so there is nothing it can truthfully report as over, and an + extraction that did reach the end has already been through + :meth:`_cleanup`. An error from the engine's close propagates. + """ logger.debug('closing %s on context exit after %d frame(s)', self._ifnm, self._frnum) # 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() + self._release() def __del__(self) -> 'None': """Release the input stream if this class still owns an open one. @@ -1797,12 +1802,13 @@ def _note_eof_progress(self) -> 'bool': previous, self._eof_mark = self._eof_mark, position return previous is None or position > previous - def _cleanup(self) -> 'None': + def _cleanup(self, *, failed: 'bool' = False) -> 'None': """Cleanup after extraction & analysis. - The method calls :meth:`self._exeng.close `, - sets :attr:`self._flag_e ` - as :data:`True` and closes the input file *if this class opened it*. + The method sets :attr:`self._flag_e ` + as :data:`True` and then calls :meth:`_release`, which closes the input + file *if this class opened it* and calls + :meth:`self._exeng.close `. That proviso is the whole of it: a handle opened here -- ``fin`` given as a path, i.e. :attr:`self._flag_s ` set -- is closed, @@ -1818,14 +1824,41 @@ def _cleanup(self) -> 'None': caller-supplied stream runs it again -- so ``finish`` is required to be idempotent rather than guarded here. + Arguments: + failed: whether the extraction stopped on an error instead, which is + the constructor raising (:issue:`2100`, :issue:`2107`). The input + and the engine are still released, but the capture has not ended, + so the flow tracer is not finished -- its callbacks would report + flows of a capture that was never read to its end -- and + :attr:`_flag_e` is not set. The constructor may have failed before + choosing an engine, as an unknown format does, and then there is + none to close. An :exc:`Exception` from the engine's close is + logged rather than raised, so it cannot replace the error that + stopped the constructor. + """ # pylint: disable=attribute-defined-outside-init logger.debug('cleaning up after %d frame(s) from %s', self._frnum, self._ifnm) - self._flag_e = True + if failed: + self._release(failed=True) + return + self._flag_e = True if self._flag_t and self._tcp: self._trace.tcp.finish() + self._release() + + def _release(self, *, failed: 'bool' = False) -> 'None': + """Close the input this class owns, and the engine. + The one close path, shared by :meth:`_cleanup` and :meth:`__exit__`, so + that the two cannot drift apart on what is released or in which order. + + Arguments: + failed: see :meth:`_cleanup`. When set, a missing engine is skipped and + an :exc:`Exception` from its close is logged rather than raised. + + """ # NOTE: *Ownership* decides who closes the input, not seekability -- # see :meth:`_owns_input` and #610. Keying on seekability # (``not self._flag_s``) gets both halves wrong at once: the handle this @@ -1835,4 +1868,20 @@ def _cleanup(self) -> 'None': if self._owns_input(): self._ifile.close() self._detach_finalizer() - self._exeng.close() + + if not failed: + self._exeng.close() + return + + # NOTE: read through ``__dict__``, as ``_owns_input`` does: a constructor + # that failed before choosing an engine leaves no ``_exeng`` (#2107). + engine = self.__dict__.get('_exeng') + if engine is None: + return + try: + engine.close() + except Exception: # pylint: disable=broad-except + # NOTE: the error that stopped the constructor is the one to report; + # letting the close's escape here would replace it (#2107). + logger.debug('engine %s failed to close after the constructor raised', + type(engine).__name__, exc_info=True) diff --git a/tests/foundation/test_extraction_constructor_closes_engine_2107_unit.py b/tests/foundation/test_extraction_constructor_closes_engine_2107_unit.py new file mode 100644 index 0000000000..cd309951c4 --- /dev/null +++ b/tests/foundation/test_extraction_constructor_closes_engine_2107_unit.py @@ -0,0 +1,261 @@ +# -*- coding: utf-8 -*- +"""An ``Extractor`` whose constructor raises after choosing an engine closes it (#2107). + +#2100 closed the input such a constructor owns, but not the engine, so an +engine's own resources -- PyShark's ``tshark`` process and temporary file -- were +left for collection. Closing it must not replace the error that stopped the +constructor, even when the close raises too. + +The constructor does that through ``_cleanup(failed=True)``, and ``__exit__`` +through the same ``_release``, so there is one close path. A failed constructor +must not finish the flow tracer, as its capture never reached its end, and may +have no engine at all. + +""" + +from __future__ import annotations + +import builtins +import importlib.util +import io +import pathlib +import struct +import tempfile +import types +import unittest +import warnings +from typing import TYPE_CHECKING +from unittest import mock + +from tests._support import reimport_once_per_class + +if TYPE_CHECKING: + from typing import Any, Callable + +#: Test tier (#1958): exercises finalisation and stream ownership. +__tier__ = 'ALL' + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) +HAS_PYSHARK = importlib.util.find_spec('pyshark') is not None + +#: A PCAP global header, little-endian, Ethernet; the fake engine never reads it. +PCAP = struct.pack(' 'types.SimpleNamespace': + """A flow tracer manager whose ``tcp.finish`` is recorded into ``calls``.""" + return types.SimpleNamespace(tcp=types.SimpleNamespace(finish=lambda: calls.append('finish'))) + + +@unittest.skipUnless(HAS_RUNTIME, 'pcapkit runtime dependencies are not installed') +class TestConstructorClosesEngine(unittest.TestCase): + """The chosen engine is closed as the constructor raises, and its error kept.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + tmp = tempfile.TemporaryDirectory() # pylint: disable=consider-using-with + self.addCleanup(tmp.cleanup) + self.path = pathlib.Path(tmp.name) / 'capture.pcap' + self.path.write_bytes(PCAP) + #: Every extractor a fake engine was built for, in order. + self.extractors = [] # type: list[Any] + + def engine(self, fail_in: 'str', close_raises: 'bool', calls: 'list[str]', + original: 'BaseException') -> 'type': + """A fake engine raising ``original`` in ``fail_in``, recording into ``calls``. + + Its ``run`` swaps in a flow tracer whose ``finish`` is recorded as well, + and keeps the extractor in :attr:`extractors` for the test to inspect. + + """ + # pylint: disable=import-outside-toplevel + from pcapkit.foundation.engines.engine import EngineBase + from pcapkit.utilities.exceptions import UnsupportedCall + + extractors = self.extractors + + class Raising(EngineBase): + name = 'raising' + module = 'raising' + + def run(self) -> None: + extractors.append(self._extractor) + self._extractor._trace = fake_trace(calls) # type: ignore[assignment] # pylint: disable=protected-access + if fail_in == 'run': + raise original + + def read_frame(self) -> None: + raise original + + def close(self) -> None: + calls.append('close') + if close_raises: + raise UnsupportedCall('close failed') + + return Raising + + @staticmethod + def selecting(engine: 'type') -> 'Callable[[Any], type]': + """A stand-in for ``Extractor._select_engine`` that picks ``engine``. + + It leaves the pcap backend probe behind as the real method does, which + ``run`` enters around the engine's constructor (#2113). + + """ + from pcapkit.foundation.engines import _pcap_backend # pylint: disable=import-outside-toplevel + + def select_engine(extractor: 'Any') -> 'type': + extractor._exprb = _pcap_backend.Memo() # pylint: disable=protected-access + return engine + + return select_engine + + @staticmethod + def recording_open() -> 'tuple[list[io.BufferedReader], Callable[..., io.BufferedReader]]': + """An :func:`open` that records the handles it returns.""" + handles = [] # type: list[io.BufferedReader] + + def recording_open(*args: 'Any', **kwargs: 'Any') -> 'io.BufferedReader': + handle = builtins.open(*args, **kwargs) # pylint: disable=consider-using-with,unspecified-encoding + handles.append(handle) + return handle + + return handles, recording_open + + def trace_kwargs(self) -> 'dict[str, Any]': + """Arguments enabling TCP flow tracing, writing under the test's own directory.""" + return {'trace': True, 'tcp': True, 'trace_format': 'json', + 'trace_fout': str(self.path.parent / 'trace')} + + def extract(self, fail_in: 'str', close_raises: 'bool' = False, **kwargs: 'Any' + ) -> 'tuple[BaseException, BaseException, list[str], list[io.BufferedReader]]': + """Extract with a fake engine raising in ``fail_in``; return what happened.""" + # pylint: disable=import-outside-toplevel + import pcapkit.foundation.extraction as extraction + from pcapkit.utilities.exceptions import FormatError + + original = FormatError(f'raised in {fail_in}') + calls = [] # type: list[str] + engine = self.engine(fail_in, close_raises, calls, original) + handles, recording_open = self.recording_open() + + with mock.patch.object(extraction.Extractor, '_select_engine', self.selecting(engine)), \ + mock.patch.object(extraction, 'open', recording_open, create=True), \ + warnings.catch_warnings(), self.assertRaises(FormatError) as caught: + warnings.simplefilter('ignore') + extraction.Extractor(fin=str(self.path), nofile=True, **kwargs) + return original, caught.exception, calls, handles + + def test_a_raising_run_closes_the_engine_and_keeps_its_error(self) -> None: + original, raised, calls, handles = self.extract('run') + self.assertIs(raised, original) + self.assertEqual(calls, ['close']) + self.assertEqual(len(handles), 1) + self.assertTrue(handles[0].closed) + + def test_a_raising_read_frame_closes_the_engine_and_keeps_its_error(self) -> None: + original, raised, calls, handles = self.extract('read_frame') + self.assertIs(raised, original) + self.assertEqual(calls, ['close']) + self.assertTrue(handles[0].closed) + + def test_a_raising_close_does_not_replace_the_original_error(self) -> None: + original, raised, calls, handles = self.extract('run', close_raises=True) + self.assertIs(raised, original) + self.assertIsNone(raised.__context__) + self.assertEqual(calls, ['close']) + self.assertTrue(handles[0].closed) + + def test_a_failed_constructor_does_not_finish_the_trace(self) -> None: + for fail_in in ('run', 'read_frame'): + with self.subTest(fail_in=fail_in): + self.extractors.clear() + original, raised, calls, handles = self.extract(fail_in, **self.trace_kwargs()) + self.assertIs(raised, original) + self.assertEqual(calls, ['close'], 'the trace was finished, or the engine left open') + self.assertTrue(handles[0].closed) + self.assertFalse(self.extractors[0]._flag_e) # pylint: disable=protected-access + + def test_an_unknown_format_cleans_up_without_an_engine(self) -> None: + # pylint: disable=import-outside-toplevel + import pcapkit.foundation.extraction as extraction + from pcapkit.utilities.exceptions import FormatError + + self.path.write_bytes(bytes(24)) + handles, recording_open = self.recording_open() + cleanup = extraction.Extractor._cleanup # pylint: disable=protected-access + with mock.patch.object(extraction.Extractor, '_cleanup', autospec=True, + side_effect=cleanup) as spy, \ + mock.patch.object(extraction, 'open', recording_open, create=True), \ + warnings.catch_warnings(), self.assertRaises(FormatError) as caught: + warnings.simplefilter('ignore') + extraction.Extractor(fin=str(self.path), nofile=True, **self.trace_kwargs()) + + self.assertIn('unknown file format', str(caught.exception)) + self.assertEqual(spy.call_count, 1) + self.assertEqual(spy.call_args.kwargs, {'failed': True}) + self.assertNotIn('_exeng', vars(spy.call_args.args[0])) + self.assertTrue(handles[0].closed) + + def test_failed_cleanup_with_no_engine_closes_only_the_input(self) -> None: + import pcapkit.foundation.extraction as extraction # pylint: disable=import-outside-toplevel + + calls = [] # type: list[str] + extractor = extraction.Extractor.__new__(extraction.Extractor) + handle = builtins.open(self.path, 'rb') # pylint: disable=consider-using-with + self.addCleanup(handle.close) + vars(extractor).update( + _ifile=handle, _ifnm=str(self.path), _flag_s=True, _flag_e=False, _flag_t=True, + _tcp=True, _frnum=0, _finalizer=None, _trace=fake_trace(calls), + ) + + extractor._cleanup(failed=True) # pylint: disable=protected-access + self.assertTrue(handle.closed) + self.assertEqual(calls, []) + self.assertFalse(extractor._flag_e) # pylint: disable=protected-access + extractor._cleanup(failed=True) # a second call is safe # pylint: disable=protected-access + + def test_exit_closes_through_the_shared_path(self) -> None: + # pylint: disable=import-outside-toplevel + import pcapkit.foundation.extraction as extraction + from pcapkit.utilities.exceptions import FormatError, UnsupportedCall + + release = extraction.Extractor._release # pylint: disable=protected-access + for close_raises in (False, True): + with self.subTest(close_raises=close_raises): + calls = [] # type: list[str] + engine = self.engine('nowhere', close_raises, calls, FormatError('unused')) + handles, recording_open = self.recording_open() + with mock.patch.object(extraction.Extractor, '_select_engine', self.selecting(engine)), \ + mock.patch.object(extraction.Extractor, '_release', autospec=True, + side_effect=release) as spy, \ + mock.patch.object(extraction, 'open', recording_open, create=True), \ + warnings.catch_warnings(): + warnings.simplefilter('ignore') + extractor: Any = extraction.Extractor(fin=str(self.path), nofile=True, auto=False, + **self.trace_kwargs()) + if close_raises: + # ``__exit__`` lets the close's error out, as it always has. + with self.assertRaises(UnsupportedCall), extractor: + pass + else: + with extractor: + pass + + spy.assert_called_once_with(extractor) + self.assertEqual(calls, ['close'], '__exit__ finished the trace, or left the engine open') + self.assertTrue(handles[0].closed) + self.assertFalse(extractor._flag_e) # pylint: disable=protected-access + + @unittest.skipUnless(HAS_PYSHARK, 'pyshark is not installed') + def test_pyshark_close_before_run_is_a_no_op(self) -> None: + from pcapkit.foundation.engines.pyshark import PyShark # pylint: disable=import-outside-toplevel + + engine = PyShark(mock.sentinel.extractor) + engine.close() + engine.close() + + +if __name__ == '__main__': + unittest.main()