diff --git a/tests/conftest.py b/tests/conftest.py index 345b6122b2..997968f4a9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -45,6 +45,7 @@ import gc import hashlib import importlib +import os import pathlib import sys import tempfile @@ -529,9 +530,10 @@ def pytest_collection_modifyitems(session: 'pytest.Session', config: 'pytest.Con #: shared hold (spanning the whole test, including the blocking #: ``subprocess.run``) would wait on an inner exclusive request that cannot be #: granted until the outer test finishes: the same deadlock shape as above, one -#: level removed. Today it never selects either, so nothing here triggers it; -#: a future change to that selection should keep it that way, or add the -#: module in question here too. +#: level removed. Today it never selects either; a future change to that +#: selection should keep it that way, or add the module in question here too. +#: The nested run's own *shared* holds are a different matter, and +#: :data:`TIER_GUARD_HELD_ENV` covers them. _LOCK_MANAGED_ELSEWHERE = frozenset({PROBE_MODULE_NAME, 'test_tier_guard_xdist.py'}) #: Whether :func:`_tier_guard_flock` polls with ``LOCK_NB`` instead of blocking @@ -550,6 +552,17 @@ def pytest_collection_modifyitems(session: 'pytest.Session', config: 'pytest.Con #: GraalPy as on CPython. TIER_GUARD_LOCK_POLL = 0.05 +#: Set, to :func:`_tier_guard_lock_path`, while :func:`_tier_guard_reader_hold` +#: holds the lock shared, so that a nested :program:`pytest` the test spawns +#: inherits it and does not ask for that lock again (#2082). The writer's gate +#: makes a second shared request wait behind any writer already queued, and the +#: writer waits on the outer hold, which waits on the nested run: a deadlock +#: that only the nested run's ``subprocess.run`` timeout ends, as +#: :mod:`tests.project.test_module_isolation` hit on GraalPy, whose slow start +#: leaves the writer time to queue. The outer hold already keeps the writer out +#: for as long as the nested run lasts, so skipping the inner one loses nothing. +TIER_GUARD_HELD_ENV = 'PCAPKIT_TEST_TIER_GUARD_HELD' + def _tier_guard_lock_path() -> 'pathlib.Path': """Where the probe/scan mutex lives, one file per checkout. @@ -659,6 +672,32 @@ def _tier_guard_lock(exclusive: 'bool') -> 'Iterator[None]': fcntl.flock(handle.fileno(), fcntl.LOCK_UN) +@contextlib.contextmanager +def _tier_guard_reader_hold() -> 'Iterator[None]': + """A test's shared hold, unless an ancestor process already has one (#2082). + + Takes :func:`_tier_guard_lock` shared and sets :data:`TIER_GUARD_HELD_ENV` + for the block, putting the variable back as it was afterwards. Where the + variable already names this lock file, the hold is an ancestor's that spans + this whole process, so the lock is not asked for again. + + """ + held = str(_tier_guard_lock_path()) + if os.environ.get(TIER_GUARD_HELD_ENV) == held: + yield + return + with _tier_guard_lock(exclusive=False): + previous = os.environ.get(TIER_GUARD_HELD_ENV) + os.environ[TIER_GUARD_HELD_ENV] = held + try: + yield + finally: + if previous is None: + os.environ.pop(TIER_GUARD_HELD_ENV, None) + else: + os.environ[TIER_GUARD_HELD_ENV] = previous + + @pytest.fixture(autouse=True) def _tier_guard_reader_lock(request: 'pytest.FixtureRequest') -> 'Iterator[None]': """Hold the probe/scan mutex shared, for every unit-tier test but the two @@ -670,7 +709,7 @@ def _tier_guard_reader_lock(request: 'pytest.FixtureRequest') -> 'Iterator[None] :class:`~tests.test_tier_guard.SuiteIsCleanTests`, so the protection is structural and covers a similarly-scanning test written later without this fixture changing at all -- and it never has to touch that module's own - source to do it. + source to do it. :func:`_tier_guard_reader_hold` takes the hold. """ location = getattr(request.node, 'path', None) or getattr(request.node, 'fspath', None) @@ -678,5 +717,5 @@ def _tier_guard_reader_lock(request: 'pytest.FixtureRequest') -> 'Iterator[None] if path is None or path.name in _LOCK_MANAGED_ELSEWHERE or not is_unit_tier(path): yield return - with _tier_guard_lock(exclusive=False): + with _tier_guard_reader_hold(): yield diff --git a/tests/test_tier_guard_xdist.py b/tests/test_tier_guard_xdist.py index a07a7c79d1..92a95d51dc 100644 --- a/tests/test_tier_guard_xdist.py +++ b/tests/test_tier_guard_xdist.py @@ -703,3 +703,95 @@ def test_a_real_leftover_still_fails_and_releases_the_lock(self) -> None: self.assertTrue(self.probe_path.exists(), 'setUp removed a probe it did not write') self._assert_lock_free() + + +#: What :class:`ProbeLockNestedRunTests` runs in a child, as a nested +#: :program:`pytest` would: take a test's shared hold on the lock file named by +#: ``sys.argv[1]``, under the environment the parent's hold handed down. +_NESTED_READER = ''' +import pathlib, sys, unittest.mock +from tests import conftest +with unittest.mock.patch.object(conftest, '_tier_guard_lock_path', + return_value=pathlib.Path(sys.argv[1])): + with conftest._tier_guard_reader_hold(): + print('IN', flush=True) +''' + + +@unittest.skipUnless(_conftest.fcntl is not None, 'fcntl unavailable on this platform') +class ProbeLockNestedRunTests(unittest.TestCase): + """A process under a test's shared hold does not ask for the lock again (#2082). + + :mod:`tests.project.test_module_isolation` runs a nested :program:`pytest` + inside its shared hold. A writer arriving meanwhile holds the gate while it + waits on that hold, so a nested shared request waited behind it, and the + hold waited on the nested run: on GraalPy the run's ``subprocess.run`` + timeout, 600 s, was what broke it, with the writer and every other reader + stalled as long. + + """ + + def setUp(self) -> None: + _redirect_tier_guard_lock(self, 'pcapkit-2082-') + patcher = unittest.mock.patch.object(_conftest, 'TIER_GUARD_LOCK_POLL', 0.01, create=True) + patcher.start() + self.addCleanup(patcher.stop) + environ = unittest.mock.patch.dict(os.environ) + environ.start() + self.addCleanup(environ.stop) + os.environ.pop(_conftest.TIER_GUARD_HELD_ENV, None) + + def _held(self, suffix: 'str', operation: 'int') -> 'bool': + """Whether ``operation`` on the lock file plus ``suffix`` would block.""" + fcntl = _conftest.fcntl + path = _conftest._tier_guard_lock_path() + with open(path.with_name(path.name + suffix), 'a', encoding='utf-8') as handle: + try: + fcntl.flock(handle.fileno(), operation | fcntl.LOCK_NB) + except BlockingIOError: + return True + fcntl.flock(handle.fileno(), fcntl.LOCK_UN) + return False + + def test_a_nested_run_does_not_queue_behind_a_waiting_writer(self) -> None: + """Fails on the code before this change: the child times out at the gate.""" + order = [] # type: list[str] + + def writer() -> None: + with _conftest._tier_guard_lock(exclusive=True): + order.append('W') + + with _conftest._tier_guard_reader_hold(): + thread = threading.Thread(target=writer, daemon=True) + thread.start() + self.addCleanup(thread.join, 10) + end = time.monotonic() + 10 + while not self._held('.gate', _conftest.fcntl.LOCK_SH): + self.assertLess(time.monotonic(), end, 'the writer never took the gate') + time.sleep(0.01) + try: + child = subprocess.run( # nosec: B603 + [sys.executable, '-c', _NESTED_READER, str(_conftest._tier_guard_lock_path())], + cwd=str(_tiers.ROOT), env=_child_environ(), capture_output=True, text=True, + timeout=120, check=False, + ) + except subprocess.TimeoutExpired: + self.fail('the nested reader queued behind the writer the outer hold keeps waiting') + self.assertEqual((child.returncode, child.stdout), (0, 'IN\n'), child.stderr) + self.assertEqual(order, [], 'the writer got in under the outer hold') + thread.join(10) + self.assertEqual(order, ['W'], 'the writer never got the lock once the hold ended') + + def test_the_variable_names_the_hold_only_while_it_lasts(self) -> None: + """Set for the hold and put back after; another lock file's value is no hold.""" + name = _conftest.TIER_GUARD_HELD_ENV + for before in (None, '/elsewhere/tier-guard.lock'): + with self.subTest(before=before): + if before is not None: + os.environ[name] = before + with _conftest._tier_guard_reader_hold(): + self.assertEqual(os.environ.get(name), str(_conftest._tier_guard_lock_path())) + self.assertTrue(self._held('', _conftest.fcntl.LOCK_EX), + 'the hold did not take the lock') + self.assertEqual(os.environ.get(name), before) + self.assertFalse(self._held('', _conftest.fcntl.LOCK_EX))