fix(foundation): back the abandoned-extraction close with weakref.finalize (#1997)

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

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__ <pcapkit.foundation.extraction.Extractor.__del__>`
+    and the :class:`weakref.finalize` held in
+    :attr:`Extractor._finalizer <pcapkit.foundation.extraction.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 <pcapkit.foundation.extraction.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()

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.