test(support): re-arm time_limit's alarm so a swallowed timeout cannot hang a worker (#1892)

tests/_support.py

@@ -88,6 +88,25 @@ def time_limit(seconds: int = 30) -> Iterator[None]:
     :data:`signal.SIGTERM` from an outer :program:`timeout`, which such a loop
     likewise never gets around to handling.
 
+    Once the deadline has passed, the :exc:`TimeoutError` repeats every second
+    until the body has left the ``with`` (#1892): each raise re-arms the alarm for
+    one more second before it is raised. One raise is not enough, because it can
+    be lost before it reaches the body. Raised inside a finaliser, it goes to
+    :func:`sys.unraisablehook` and is dropped; an ``except OSError`` drops it too,
+    :exc:`TimeoutError` being one, and that is exactly what
+    :meth:`Extractor.__del__ <pcapkit.foundation.extraction.Extractor.__del__>`
+    catches. With a single alarm the body then runs on until pytest-timeout's
+    backstop ends the whole :program:`pytest-xdist` worker, with no traceback --
+    what the option generator's own one-shot deadline did (#1889). The repeat
+    stops on the way out, whichever way the body leaves.
+
+    One window stays open, and pure Python cannot close it: a deadline that comes
+    due after the body has ended but before the generator resumes, inside
+    :meth:`contextlib._GeneratorContextManager.__exit__`, raises
+    :exc:`TimeoutError` into the caller before the way out has run. The repeat
+    then keeps firing until that exception is released, since its traceback holds
+    the generator whose ``finally`` would stop it.
+
     There is only ever one pending alarm per process, so arming this one cancels
     whatever was already scheduled -- an enclosing ``time_limit``, or a deadline the
     test runner set for itself. Both the handler and that pending alarm are put back
@@ -120,7 +139,7 @@ def time_limit(seconds: int = 30) -> Iterator[None]:
 
     Raises:
         TimeoutError: If the body has not finished within ``seconds`` seconds,
-            as scaled.
+            as scaled -- and again each second after that, until it has.
         ValueError: If :data:`TIMEOUT_SCALE_ENV` is malformed; see
             :func:`scale_timeout`.
 
@@ -134,7 +153,18 @@ def time_limit(seconds: int = 30) -> Iterator[None]:
     # Before the handler is swapped, so a malformed scale leaves nothing to undo.
     seconds = scale_timeout(seconds)
 
-    def expire(signum: int, frame: object) -> None:
+    # Set as the first statement on the way out: from then on a repeat already
+    # due does nothing, so it cannot interrupt the restoring below or re-arm
+    # itself past the ``with``. A plain store, not a call, so that a repeat
+    # landing right at the way out has no extra place to be delivered.
+    leaving = False
+
+    def expire(signum: int, frame: object) -> None:  # pylint: disable=unused-argument
+        if leaving:
+            return
+        # Re-armed before raising, so a raise that never reaches the body is
+        # followed by another one (#1892).
+        signal.alarm(1)
         raise TimeoutError(f'did not finish within {seconds}s')
 
     previous_handler = signal.signal(signal.SIGALRM, expire)
@@ -154,12 +184,14 @@ def expire(signum: int, frame: object) -> None:
         started = time.monotonic()
         yield
     finally:
-        # Cancel first, so that an alarm which fires between here and the handler
-        # being restored cannot be delivered to whatever handler was installed
-        # before -- and so that the alarm re-armed below belongs to that handler
-        # rather than to ``expire``. Only if arming happened, though: a failed
-        # ``signal.alarm`` replaced nothing, so an enclosing deadline is still
-        # pending as it was, and cancelling it here would drop it unrecorded.
+        leaving = True
+        # Cancel first -- the deadline, or the repeat it has re-armed -- so that
+        # an alarm which fires between here and the handler being restored cannot
+        # be delivered to whatever handler was installed before, and so that the
+        # alarm re-armed below belongs to that handler rather than to ``expire``.
+        # Only if arming happened, though: a failed ``signal.alarm`` replaced
+        # nothing, so an enclosing deadline is still pending as it was, and
+        # cancelling it here would drop it unrecorded.
         if armed:
             signal.alarm(0)
         signal.signal(signal.SIGALRM, previous_handler)

tests/test_support_helpers.py

@@ -53,6 +53,7 @@
 """
 from __future__ import annotations
 
+import contextlib
 import hashlib
 import importlib
 import importlib.util
@@ -76,6 +77,11 @@
                             skip_if_stand_in, snapshot_modules, stand_in_reason, time_limit,
                             upstream_digests)
 
+#: Seconds a :class:`TimeLimitTests` body spins waiting for the deadline before it
+#: gives up and fails by name: ten times the one-second deadline those tests arm,
+#: so a deadline that fires at all, its repeat included, lands well inside it.
+SPIN_LIMIT = 10.0
+
 
 class Closeable:
     """A stand-in for a resource that records having been closed.
@@ -226,8 +232,30 @@ class TimeLimitTests(unittest.TestCase):
     pin that, because an enclosing deadline going missing is invisible until the
     run it should have bounded hangs instead.
 
+    A deadline that has fired repeats until the ``with`` is left (#1892), since its
+    first raise can be dropped before the body sees it; these pin that the repeat
+    gets through and that it never outlives the block. Every body that waits for
+    the deadline here gives up after :data:`SPIN_LIMIT` and fails by name, so a
+    deadline that never gets through fails this test in seconds rather than
+    hanging the worker until pytest-timeout's backstop ends it.
+
     """
 
+    def spin(self, until: float) -> None:
+        """Busy-wait in pure Python until :func:`time.monotonic` reaches ``until``.
+
+        The stand-in for a hang, but a bounded one. Pure Python, so the deadline's
+        signal is delivered inside it, between bytecodes.
+
+        """
+        while time.monotonic() < until:
+            pass
+
+    def hang(self, until: float) -> None:
+        """:meth:`spin`, then fail: the deadline should have interrupted it first."""
+        self.spin(until)
+        self.fail(f'time_limit did not interrupt the body within {SPIN_LIMIT:g}s')
+
     def setUp(self) -> None:
         # Whatever a test leaves behind, the next one starts from nothing pending
         # and from a handler this class owns rather than the helper's.
@@ -247,8 +275,7 @@ def test_the_deadline_fires_on_a_body_that_overruns(self) -> None:
         """The point of the helper, pinned so the rest cannot be met by disarming."""
         with self.assertRaises(TimeoutError):
             with time_limit(1):
-                while True:
-                    pass
+                self.hang(time.monotonic() + SPIN_LIMIT)
 
     def test_an_enclosing_alarm_is_restored(self) -> None:
         """An outer deadline keeps counting down across the ``with``.
@@ -320,8 +347,7 @@ def test_the_timeout_message_names_the_scaled_deadline(self) -> None:
         os.environ[TIMEOUT_SCALE_ENV] = '0.5'
         with self.assertRaisesRegex(TimeoutError, r'within 1s'):
             with time_limit(2):
-                while True:
-                    pass
+                self.hang(time.monotonic() + SPIN_LIMIT)
 
     def test_a_scaled_deadline_rounds_up_to_whole_seconds(self) -> None:
         """An alarm counts whole seconds, and a deadline of zero is no deadline."""
@@ -384,6 +410,148 @@ def alarm(seconds: int) -> int:
         self.assertIs(signal.getsignal(signal.SIGALRM), own_handler)
         self.assertIn(real_alarm(0), range(28, 31))
 
+    def assert_nothing_left_behind(self, own_handler: 'object') -> None:
+        """No alarm pending, this class's handler back, and nothing delivered to it."""
+        self.assertIs(signal.getsignal(signal.SIGALRM), own_handler)
+        self.assertEqual(signal.alarm(0), 0)
+        self.assertEqual(self.handled, [])
+
+    def test_each_raise_re_arms_the_alarm_for_one_second(self) -> None:
+        """The handler re-arms before it raises, so a dropped raise has a successor."""
+        with time_limit(5):
+            expire = signal.getsignal(signal.SIGALRM)
+            with self.assertRaisesRegex(TimeoutError, r'within 5s'):
+                expire(signal.SIGALRM, None)
+            self.assertEqual(signal.alarm(0), 1)
+
+    @unittest.skipUnless(sys.implementation.name == 'cpython' and sys.version_info >= (3, 8),
+                         'needs a finaliser run on the last reference, and sys.unraisablehook')
+    def test_a_raise_a_finaliser_drops_is_raised_again(self) -> None:
+        """#1892: the first raise lands in a finaliser and is lost; the repeat is not.
+
+        A finaliser cannot raise: what escapes one goes to
+        :func:`sys.unraisablehook` and the code that dropped the last reference
+        carries on. So the body below outlives its deadline unless the deadline
+        tries again.
+
+        """
+        own_handler = signal.getsignal(signal.SIGALRM)
+        end = time.monotonic() + SPIN_LIMIT
+        dropped = []  # type: list[type]
+        spin = self.spin
+
+        class Finalised:  # pylint: disable=too-few-public-methods
+            """Holds the body in its finaliser until the deadline lands there."""
+
+            def __del__(self) -> None:
+                spin(end)
+
+        with mock.patch.object(sys, 'unraisablehook',
+                               lambda unraisable: dropped.append(unraisable.exc_type)):
+            with self.assertRaisesRegex(TimeoutError, r'within 1s'):
+                with time_limit(1):
+                    held = Finalised()
+                    del held
+                    self.hang(end)
+
+        self.assertEqual(dropped, [TimeoutError])
+        self.assert_nothing_left_behind(own_handler)
+
+    def test_a_raise_an_except_oserror_drops_is_raised_again(self) -> None:
+        """#1892: :exc:`TimeoutError` is an :exc:`OSError`, so a handler for one drops it.
+
+        :meth:`pcapkit.foundation.extraction.Extractor.__del__` guards its close
+        with exactly that clause, so the first raise is caught there and the body
+        carries on, as it does below.
+
+        """
+        own_handler = signal.getsignal(signal.SIGALRM)
+        end = time.monotonic() + SPIN_LIMIT
+        dropped = []  # type: list[type]
+
+        with self.assertRaisesRegex(TimeoutError, r'within 1s'):
+            with time_limit(1):
+                try:
+                    self.hang(end)
+                except OSError as exc:
+                    dropped.append(type(exc))
+                self.hang(end)
+
+        self.assertEqual(dropped, [TimeoutError])
+        self.assert_nothing_left_behind(own_handler)
+
+    def test_the_repeat_never_outlives_the_block(self) -> None:
+        """Left normally, by timeout, or by another exception, nothing stays armed.
+
+        Each exit is taken while a repeat is pending: the first raise is caught in
+        the body, as a dropped one would be, so the alarm the handler re-armed is
+        still due when the ``with`` is left.
+
+        """
+        own_handler = signal.getsignal(signal.SIGALRM)
+        for how, expected in (('normally', None), ('by timeout', TimeoutError),
+                              ('by another exception', LookupError)):
+            with self.subTest(exit=how):
+                end = time.monotonic() + SPIN_LIMIT
+                raised = None
+                try:
+                    with time_limit(1):
+                        with contextlib.suppress(TimeoutError):
+                            self.hang(end)
+                        if expected is TimeoutError:
+                            self.hang(end)
+                        elif expected is LookupError:
+                            raise LookupError(how)
+                except (TimeoutError, LookupError) as exc:
+                    raised = type(exc)
+
+                self.assertIs(raised, expected)
+                self.assert_nothing_left_behind(own_handler)
+
+    def test_a_repeat_due_as_the_block_is_left_does_nothing(self) -> None:
+        """A deadline landing while the way out runs neither raises nor re-arms.
+
+        The signal is sent from inside the cancel, while ``time_limit``'s own
+        handler is still installed: the point a repeat already due would land.
+        Raised there, it would escape the ``with`` and leave that handler and a
+        fresh repeat behind it.
+
+        """
+        own_handler = signal.getsignal(signal.SIGALRM)
+        real_alarm = signal.alarm
+        sent = []  # type: list[int]
+
+        def alarm(seconds: int) -> int:
+            if seconds == 0:
+                sent.append(signal.SIGALRM)
+                os.kill(os.getpid(), signal.SIGALRM)
+            return real_alarm(seconds)
+
+        with mock.patch.object(signal, 'alarm', side_effect=alarm):
+            with time_limit(5):
+                pass
+
+        self.assertEqual(sent, [signal.SIGALRM])
+        self.assert_nothing_left_behind(own_handler)
+
+    def test_an_enclosing_alarm_survives_a_deadline_that_repeated(self) -> None:
+        """An outer deadline comes back, less the body's seconds, after a repeat."""
+        own_handler = signal.getsignal(signal.SIGALRM)
+        end = time.monotonic() + SPIN_LIMIT
+
+        signal.alarm(30)
+        with self.assertRaisesRegex(TimeoutError, r'within 1s'):
+            with time_limit(1):
+                with contextlib.suppress(TimeoutError):
+                    self.hang(end)
+                self.hang(end)
+
+        # The body ran for the deadline and its repeat at least, and for less than
+        # :data:`SPIN_LIMIT`, or it would have failed rather than timed out.
+        self.assertIn(signal.alarm(0), range(30 - int(SPIN_LIMIT), 29))
+        self.assertIs(signal.getsignal(signal.SIGALRM), own_handler)
+        self.assertEqual(self.handled, [])
+
 
 class SnapshotRestoreTests(unittest.TestCase):
     """:func:`~tests._support.restore_modules` is an exact inverse.