From a525629f1573e3dba45e5e18d15107a58853ccd8 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sat, 17 Oct 2026 02:27:59 -0400 Subject: [PATCH] test(support): re-arm time_limit's alarm so a swallowed timeout cannot hang a worker (#1892) - time_limit's handler re-arms the alarm for one second before each raise, so a TimeoutError dropped by a finaliser or an `except OSError` is followed by another until the body leaves the `with`. - A flag set first on the way out turns a repeat already due into a no-op; the repeat is cancelled and the previous handler and pending alarm are restored on every exit, as before. The docstring names the window inside contextlib's `__exit__` that pure Python cannot close. - TimeLimitTests pins the repeat through a finaliser and an `except OSError`, that a repeat landing on the way out does nothing, that nothing stays armed after any exit, and that an enclosing alarm survives a repeat. - Every body there that waits for the deadline is bounded and fails by name, so a lost deadline fails in seconds instead of hanging the worker. Closes #1892 --- tests/_support.py | 48 ++++++++-- tests/test_support_helpers.py | 176 +++++++++++++++++++++++++++++++++- 2 files changed, 212 insertions(+), 12 deletions(-) diff --git a/tests/_support.py b/tests/_support.py index 9ce7010a0..67378cfaa 100644 --- a/tests/_support.py +++ b/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__ ` + 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) diff --git a/tests/test_support_helpers.py b/tests/test_support_helpers.py index 7f66abee7..ae8c17385 100644 --- a/tests/test_support_helpers.py +++ b/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.