diff --git a/docs/source/pcapkit/foundation/extraction.rst b/docs/source/pcapkit/foundation/extraction.rst index 8f42e24c99..01eea3eb7b 100644 --- a/docs/source/pcapkit/foundation/extraction.rst +++ b/docs/source/pcapkit/foundation/extraction.rst @@ -72,6 +72,7 @@ and extracts parameters from a PCAP file. .. autoattribute:: _flag_s .. autoattribute:: _ifile + .. autoattribute:: _finalizer .. autoattribute:: _ofile .. autoattribute:: _ifnm .. autoattribute:: _ofnm diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index 3a9f69b07d..286222533e 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -20,6 +20,7 @@ import os import sys import types +import weakref from typing import TYPE_CHECKING, Generic, TypeVar, cast from dictdumper.dumper import Dumper @@ -106,6 +107,35 @@ _PATH_ENGINES = frozenset({'scapy', 'pyshark', 'pypcap', 'pcap_ct'}) +def _release_input(ifile: 'IO[bytes]') -> 'None': + """Close an input stream from a finaliser, if it is still open. + + The one close both backstops of :issue:`606` perform -- + :meth:`Extractor.__del__ ` + and the :class:`weakref.finalize` held in + :attr:`Extractor._finalizer `. + It takes the stream rather than the extractor, so that a finaliser built on + it holds no reference that would keep the extractor alive; deciding whether + the stream is the extractor's to close is the caller's business, cf. + :meth:`Extractor._owns_input `. + + Args: + ifile: The stream to close. + + Note: + A finaliser must not raise, so the close is guarded: that is what a close + during interpreter shutdown can fail at. + + """ + try: + if not ifile.closed: + ifile.close() + except (OSError, ValueError): # pragma: no cover + # Nothing useful can be reported from a finaliser, and raising here + # would only produce an "Exception ignored in __del__" on stderr. + pass + + class Extractor(Generic[_P]): """Extractor for PCAP files. @@ -229,6 +259,12 @@ class Extractor(Generic[_P]): #: Input file object. _ifile: 'BufferedReader' + #: Backstop that closes :attr:`_ifile` when the extractor is collected + #: before :meth:`_cleanup` has run, registered only when the stream is + #: this class's own to close (:issue:`1997`), else :data:`None`. + #: :meth:`__del__` alone is not enough, as GraalPy never calls it; a + #: :class:`weakref.finalize` runs there and on CPython alike. + _finalizer: 'Optional[weakref.finalize]' #: Output writer. It holds the writer class until :meth:`_open_output` #: replaces it with the writer, or under split mode with a factory. _ofile: 'Dumper | Type[Dumper]' @@ -1439,6 +1475,15 @@ def __init__(self, logger.debug('input stream has no peek(), wrapping it in PeekableStream') self._ifile = cast('BufferedReader', PeekableStream(self._ifile)) + # NOTE: the #606 backstop for an extraction abandoned before EOF. GraalPy + # never calls ``__del__``, so it is a ``weakref.finalize`` as well (#1997). + # It closes over the stream alone -- a reference to ``self`` would keep + # the extractor alive for as long as the finaliser is -- and it is only + # registered for a stream this class owns, so a caller's is never closed. + self._finalizer = None + if self._owns_input(): + self._finalizer = weakref.finalize(self, _release_input, self._ifile) + if not self._flag_q: output, ext = self.__output__[fmt] if ext is None: @@ -1548,6 +1593,7 @@ def __exit__(self, exc_type: 'Type[BaseException] | None', exc_value: 'BaseExcep """Close the input file when exits.""" logger.debug('closing %s on context exit after %d frame(s)', self._ifnm, self._frnum) self._ifile.close() + self._detach_finalizer() self._exeng.close() def __del__(self) -> 'None': @@ -1561,6 +1607,13 @@ def __del__(self) -> 'None': ``ResourceWarning`` raised wherever collection happens to run, which can be far from the code that abandoned the handle (:issue:`606`). + GraalPy never calls this method, so the same close is also registered as + the :class:`weakref.finalize` in :attr:`_finalizer`, which runs on either + interpreter (:issue:`1997`). This method stays for whoever calls it + directly, a subclass extending it through :func:`super` among them, and + both are safe to run for one stream: whichever comes second finds it + already closed. + This is a backstop and not the recommended route: collection is not deterministic, and :class:`Extractor` takes part in a reference cycle through its engine, so a handle can outlive its last reference until the @@ -1580,14 +1633,7 @@ def __del__(self) -> 'None': """ if not self._owns_input(): return - try: - ifile = self.__dict__['_ifile'] - if not ifile.closed: - ifile.close() - except (OSError, ValueError): # pragma: no cover - # Nothing useful can be reported from a finaliser, and raising here - # would only produce an "Exception ignored in __del__" on stderr. - pass + _release_input(self.__dict__['_ifile']) ########################################################################## # Utilities. @@ -1597,7 +1643,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` and :meth:`__del__` cannot drift apart on it. + :meth:`_cleanup`, :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 @@ -1633,6 +1680,20 @@ def _owns_input(self) -> 'bool': return True return isinstance(ifile, SeekableReader) + def _detach_finalizer(self) -> 'None': + """Unregister :attr:`_finalizer`, once the input has been closed on purpose. + + A finished extraction then leaves nothing registered against its stream: + neither a second close at collection nor a reference that keeps the stream + alive until the extractor is collected. + + """ + # NOTE: read through ``__dict__``, as ``_owns_input`` does, for an + # instance whose constructor did not get as far as registering one. + finalizer = self.__dict__.get('_finalizer') + if finalizer is not None: + finalizer.detach() + def _note_eof_progress(self) -> 'bool': """Record this end of stream, and say whether the input advanced to reach it. @@ -1734,4 +1795,5 @@ def _cleanup(self) -> 'None': # supplied and still needs *is* closed. if self._owns_input(): self._ifile.close() + self._detach_finalizer() self._exeng.close() diff --git a/tests/foundation/test_extraction_ownership.py b/tests/foundation/test_extraction_ownership.py index e827559e88..3c965bc88e 100644 --- a/tests/foundation/test_extraction_ownership.py +++ b/tests/foundation/test_extraction_ownership.py @@ -337,6 +337,131 @@ def test_non_seekable_stream_survives_its_seekable_wrapper(self) -> None: stream.close() +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class FinaliserBackstopWithoutDel(unittest.TestCase): + """#1997: the backstop is a :class:`weakref.finalize`, not ``__del__`` alone. + + GraalPy never calls ``__del__``, so on that interpreter the #606 backstop + only ever ran if something else did the closing. CPython does call it, which + is why a regression here would go unnoticed on every CPython leg unless the + finaliser is checked on its own: these tests do that, either by inspecting it + or by taking ``__del__`` out of the way, so that they fail on CPython too. + + """ + + def test_abandoned_extraction_registers_a_finaliser_over_its_own_handle(self) -> None: + """The finaliser is alive mid-extraction and holds the stream, not ``self``. + + A finaliser that referenced the extractor would keep it alive for as long + as the finaliser is, which is to say for ever. + + """ + import weakref + + import pcapkit + + extractor = pcapkit.extract(fin=sample_path('in.pcap'), nofile=True, + store=False, auto=False) + next(extractor) + + finalizer = extractor._finalizer + self.assertIsInstance(finalizer, weakref.finalize) + self.assertTrue(finalizer.alive, + 'an extraction still in flight should have its backstop ' + 'registered') + + target, _, args, kwargs = finalizer.peek() + self.assertIs(target, extractor) + self.assertEqual(len(args), 1) + self.assertIs(args[0], extractor._ifile) + self.assertFalse(any(value is extractor for value in kwargs.values()), + 'the finaliser must not hold the extractor itself') + + self.assertFalse(extractor._ifile.closed) + finalizer() + self.assertTrue(extractor._ifile.closed, + 'invoking the finaliser should release the handle') + self.assertFalse(finalizer.alive) + + def test_finaliser_alone_releases_an_abandoned_handle(self) -> None: + """With ``__del__`` a no-op, as on GraalPy, collection still closes the file.""" + import pcapkit + from pcapkit.foundation.extraction import Extractor + + capture = sample_path('in.pcap') + recorder = _OpenRecorder(capture) + with mock.patch.object(Extractor, '__del__', lambda self: None): + with mock.patch.object(builtins, 'open', recorder): + extractor = pcapkit.extract(fin=capture, nofile=True, store=False, + auto=False) + next(extractor) + self.assertEqual(recorder.still_open, 1) + + del extractor + gc.collect() + + self.assertEqual(recorder.still_open, 0, + 'without __del__ the handle outlived the extractor') + + def test_finished_extraction_detaches_its_finaliser(self) -> None: + """``_cleanup`` and the context manager unregister it, so nothing closes twice.""" + import pcapkit + + capture = sample_path('in.pcap') + with self.subTest(route='_cleanup at end of file'): + extractor = pcapkit.extract(fin=capture, nofile=True, store=False) + self.assertTrue(extractor._ifile.closed) + self.assertFalse(extractor._finalizer.alive) + + with self.subTest(route='context manager exit'): + with pcapkit.extract(fin=capture, nofile=True, store=False, + auto=False) as extractor: + next(extractor) + self.assertTrue(extractor._finalizer.alive) + self.assertTrue(extractor._ifile.closed) + self.assertFalse(extractor._finalizer.alive) + + def test_no_finaliser_for_a_callers_stream(self) -> None: + """A stream the caller passed in gets no finaliser, so collection leaves it open.""" + import pcapkit + from pcapkit.foundation.extraction import Extractor + + with open(sample_path('in.pcap'), 'rb') as handle: + with mock.patch.object(Extractor, '__del__', lambda self: None): + extractor = pcapkit.extract(fin=handle, nofile=True, store=False, + auto=False) + next(extractor) + self.assertIsNone(extractor._finalizer) + + del extractor + gc.collect() + + self.assertFalse(handle.closed, + "collecting the extractor closed the caller's stream") + + def test_finaliser_on_a_wrapper_spares_the_stream_beneath(self) -> None: + """A wrapped stream's finaliser closes the wrapper, not the caller's stream.""" + import pcapkit + from pcapkit.corekit.io import SeekableReader + + with open(sample_path('in.pcap'), 'rb') as source: + payload = source.read() + stream = io.BufferedReader(_NonSeekableRaw(payload)) + self.addCleanup(stream.close) + + extractor = pcapkit.extract(fin=stream, nofile=True, store=False, + auto=False) + next(extractor) + self.assertIsInstance(extractor._ifile, SeekableReader) + self.assertTrue(extractor._finalizer.alive) + + extractor._finalizer() + self.assertTrue(extractor._ifile.closed) + self.assertFalse(stream.closed, + "the finaliser closed the caller's stream through the " + 'wrapper') + + class OwnershipRuleInIsolation(unittest.TestCase): """``_owns_input`` and ``__del__`` without constructing a real extraction.