From 3bd67a50334d722f38f9a900973e31062272da00 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 6 Sep 2024 12:34:52 +0200 Subject: [PATCH 01/18] initial implementation of lot breaking --- src/trio/_core/__init__.py | 7 +++++- src/trio/_core/_parking_lot.py | 41 ++++++++++++++++++++++++++++++++++ src/trio/_core/_run.py | 6 +++++ src/trio/_sync.py | 4 ++++ src/trio/_tests/test_sync.py | 24 ++++++++++++++++++++ src/trio/lowlevel.py | 2 ++ 6 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/trio/_core/__init__.py b/src/trio/_core/__init__.py index 71f5f17eb2..fdef90292d 100644 --- a/src/trio/_core/__init__.py +++ b/src/trio/_core/__init__.py @@ -20,7 +20,12 @@ from ._ki import currently_ki_protected, disable_ki_protection, enable_ki_protection from ._local import RunVar, RunVarToken from ._mock_clock import MockClock -from ._parking_lot import ParkingLot, ParkingLotStatistics +from ._parking_lot import ( + ParkingLot, + ParkingLotStatistics, + add_parking_lot_breaker, + remove_parking_lot_breaker, +) # Imports that always exist from ._run import ( diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index 916e6a6e96..ea5c76ba56 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -76,6 +76,7 @@ from typing import TYPE_CHECKING import attrs +import outcome from .. import _core from .._util import final @@ -86,6 +87,24 @@ from ._run import Task +GLOBAL_PARKING_LOT_BREAKER: dict[Task, list[ParkingLot]] = {} + + +def add_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: + if task not in GLOBAL_PARKING_LOT_BREAKER: + GLOBAL_PARKING_LOT_BREAKER[task] = [lot] + else: + GLOBAL_PARKING_LOT_BREAKER[task].append(lot) + + +def remove_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: + if task not in GLOBAL_PARKING_LOT_BREAKER: + raise RuntimeError( + "Attempted to remove parking lot breaker for task that is not registered as a breaker", + ) + GLOBAL_PARKING_LOT_BREAKER[task].remove(lot) + + @attrs.frozen class ParkingLotStatistics: """An object containing debugging information for a ParkingLot. @@ -118,6 +137,7 @@ class ParkingLot: # {task: None}, we just want a deque where we can quickly delete random # items _parked: OrderedDict[Task, None] = attrs.field(factory=OrderedDict, init=False) + broken_by: Task | None = None def __len__(self) -> int: """Returns the number of parked tasks.""" @@ -137,6 +157,10 @@ async def park(self) -> None: :meth:`unpark_all`. """ + if self.broken_by is not None: + raise _core.BrokenResourceError( + f"Attempted to park in parking lot broken by {self.broken_by}", + ) task = _core.current_task() self._parked[task] = None task.custom_sleep_data = self @@ -234,6 +258,23 @@ def repark_all(self, new_lot: ParkingLot) -> None: """ return self.repark(new_lot, count=len(self)) + def break_lot(self, task: Task) -> None: + """Break this lot, causing all parked tasks to raise an error, and any + future tasks attempting to park (and unpark? repark?) to error. The error + contains a reference to the task sent as a parameter.""" + self.broken_by = task + # TODO: weird to phrase this one, we probably should reraise this error in Lock + error = outcome.Error( + _core.BrokenResourceError(f"Parking lot broken by {task}"), + ) + + # TODO: is there any reason to use self._pop_several? + for parked_task in self._parked: + if parked_task is task: + continue + _core.reschedule(parked_task, error) + self._parked.clear() + def statistics(self) -> ParkingLotStatistics: """Return an object containing debugging information. diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 8921ed7f12..dfb2064832 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -40,6 +40,7 @@ from ._exceptions import Cancelled, RunFinishedError, TrioInternalError from ._instrumentation import Instruments from ._ki import LOCALS_KEY_KI_PROTECTION_ENABLED, KIManager, enable_ki_protection +from ._parking_lot import GLOBAL_PARKING_LOT_BREAKER from ._thread_cache import start_thread_soon from ._traps import ( Abort, @@ -1859,6 +1860,11 @@ def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: assert task._parent_nursery is not None, task task._parent_nursery._child_finished(task, outcome) + # before or after the other stuff in this function? + if task in GLOBAL_PARKING_LOT_BREAKER: + for lot in GLOBAL_PARKING_LOT_BREAKER[task]: + lot.break_lot(task) + if "task_exited" in self.instruments: self.instruments.call("task_exited", task) diff --git a/src/trio/_sync.py b/src/trio/_sync.py index 698716ea35..d08da9f302 100644 --- a/src/trio/_sync.py +++ b/src/trio/_sync.py @@ -6,6 +6,7 @@ import attrs import trio +from trio.lowlevel import add_parking_lot_breaker, remove_parking_lot_breaker from . import _core from ._core import Abort, ParkingLot, RaiseCancelT, enable_ki_protection @@ -576,6 +577,7 @@ def acquire_nowait(self) -> None: elif self._owner is None and not self._lot: # No-one owns it self._owner = task + add_parking_lot_breaker(task, self._lot) else: raise trio.WouldBlock @@ -604,8 +606,10 @@ def release(self) -> None: task = trio.lowlevel.current_task() if task is not self._owner: raise RuntimeError("can't release a Lock you don't own") + remove_parking_lot_breaker(self._owner, self._lot) if self._lot: (self._owner,) = self._lot.unpark(count=1) + add_parking_lot_breaker(self._owner, self._lot) else: self._owner = None diff --git a/src/trio/_tests/test_sync.py b/src/trio/_tests/test_sync.py index caf3f04f5b..92fd6beec4 100644 --- a/src/trio/_tests/test_sync.py +++ b/src/trio/_tests/test_sync.py @@ -5,6 +5,8 @@ import pytest +from trio.testing import Matcher, RaisesGroup + from .. import _core from .._sync import * from .._timeouts import sleep_forever @@ -586,3 +588,25 @@ async def lock_taker() -> None: await wait_all_tasks_blocked() assert record == ["started"] lock_like.release() + + +async def test_lock_acquire_unowned_lock() -> None: + """Test that trying to acquire a lock whose owner has exited raises an error. + Partial fix for https://github.com/python-trio/trio/issues/3035 + """ + lock = trio.Lock() + async with trio.open_nursery() as nursery: + nursery.start_soon(lock.acquire) + with pytest.raises( + trio.BrokenResourceError, + match="^Attempted to park in parking lot broken by", + ): + await lock.acquire() + + +async def test_lock_multiple_acquire() -> None: + lock = trio.Lock() + with RaisesGroup(Matcher(trio.BrokenResourceError, match="Parking lot broken by")): + async with trio.open_nursery() as nursery: + nursery.start_soon(lock.acquire) + nursery.start_soon(lock.acquire) diff --git a/src/trio/lowlevel.py b/src/trio/lowlevel.py index 1df7019637..f532ae6367 100644 --- a/src/trio/lowlevel.py +++ b/src/trio/lowlevel.py @@ -25,6 +25,7 @@ UnboundedQueue as UnboundedQueue, UnboundedQueueStatistics as UnboundedQueueStatistics, add_instrument as add_instrument, + add_parking_lot_breaker, cancel_shielded_checkpoint as cancel_shielded_checkpoint, checkpoint as checkpoint, checkpoint_if_cancelled as checkpoint_if_cancelled, @@ -40,6 +41,7 @@ permanently_detach_coroutine_object as permanently_detach_coroutine_object, reattach_detached_coroutine_object as reattach_detached_coroutine_object, remove_instrument as remove_instrument, + remove_parking_lot_breaker, reschedule as reschedule, spawn_system_task as spawn_system_task, start_guest_run as start_guest_run, From 4dfa1ad214f68e02e2e61c27df3b994740492da8 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 6 Sep 2024 12:42:02 +0200 Subject: [PATCH 02/18] fix import cycle --- src/trio/_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/trio/_sync.py b/src/trio/_sync.py index d08da9f302..8e191d00fb 100644 --- a/src/trio/_sync.py +++ b/src/trio/_sync.py @@ -6,10 +6,10 @@ import attrs import trio -from trio.lowlevel import add_parking_lot_breaker, remove_parking_lot_breaker from . import _core from ._core import Abort, ParkingLot, RaiseCancelT, enable_ki_protection +from ._core._parking_lot import add_parking_lot_breaker, remove_parking_lot_breaker from ._util import final if TYPE_CHECKING: From 543a087fd007ce1094e9e9f0a87b30b3a6ffd387 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 6 Sep 2024 12:47:44 +0200 Subject: [PATCH 03/18] fix re-export for verifytypes visibility --- src/trio/lowlevel.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/trio/lowlevel.py b/src/trio/lowlevel.py index f532ae6367..9e385a0045 100644 --- a/src/trio/lowlevel.py +++ b/src/trio/lowlevel.py @@ -25,7 +25,7 @@ UnboundedQueue as UnboundedQueue, UnboundedQueueStatistics as UnboundedQueueStatistics, add_instrument as add_instrument, - add_parking_lot_breaker, + add_parking_lot_breaker as add_parking_lot_breaker, cancel_shielded_checkpoint as cancel_shielded_checkpoint, checkpoint as checkpoint, checkpoint_if_cancelled as checkpoint_if_cancelled, @@ -41,7 +41,7 @@ permanently_detach_coroutine_object as permanently_detach_coroutine_object, reattach_detached_coroutine_object as reattach_detached_coroutine_object, remove_instrument as remove_instrument, - remove_parking_lot_breaker, + remove_parking_lot_breaker as remove_parking_lot_breaker, reschedule as reschedule, spawn_system_task as spawn_system_task, start_guest_run as start_guest_run, From c36cdad13424f6795e8b819f410487fdb3eb16f9 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 6 Sep 2024 12:58:38 +0200 Subject: [PATCH 04/18] update docstrings --- src/trio/_core/_parking_lot.py | 8 ++++++++ src/trio/_sync.py | 6 +++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index ea5c76ba56..c809a445ef 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -91,6 +91,9 @@ def add_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: + """Register a task as a breaker for a lot. This means that if the task exits without + having unparked from the lot, then the lot will break and raise an error for all tasks + parked in the lot, as well as any future task that attempt to park in it.""" if task not in GLOBAL_PARKING_LOT_BREAKER: GLOBAL_PARKING_LOT_BREAKER[task] = [lot] else: @@ -98,6 +101,7 @@ def add_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: def remove_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: + """Deregister a task as a breaker for a lot. See :func:`add_parking_lot_breaker`.""" if task not in GLOBAL_PARKING_LOT_BREAKER: raise RuntimeError( "Attempted to remove parking lot breaker for task that is not registered as a breaker", @@ -156,6 +160,10 @@ async def park(self) -> None: """Park the current task until woken by a call to :meth:`unpark` or :meth:`unpark_all`. + Raises: + BrokenResourceError: if attempting to park in a broken lot, or the lot + breaks before we get to unpark. + """ if self.broken_by is not None: raise _core.BrokenResourceError( diff --git a/src/trio/_sync.py b/src/trio/_sync.py index 8e191d00fb..9a812f68a4 100644 --- a/src/trio/_sync.py +++ b/src/trio/_sync.py @@ -583,7 +583,11 @@ def acquire_nowait(self) -> None: @enable_ki_protection async def acquire(self) -> None: - """Acquire the lock, blocking if necessary.""" + """Acquire the lock, blocking if necessary. + + Raises: + BrokenResourceError: if the owner of the lock exits without releasing. + """ await trio.lowlevel.checkpoint_if_cancelled() try: self.acquire_nowait() From 127c5fcaef2420bd5d8c3992792b4fedf0c0e376 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 6 Sep 2024 13:15:16 +0200 Subject: [PATCH 05/18] fixes after review by TeamSpen210 --- src/trio/_core/_parking_lot.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index c809a445ef..f515a77291 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -102,11 +102,12 @@ def add_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: def remove_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: """Deregister a task as a breaker for a lot. See :func:`add_parking_lot_breaker`.""" - if task not in GLOBAL_PARKING_LOT_BREAKER: + try: + GLOBAL_PARKING_LOT_BREAKER[task].remove(lot) + except (KeyError, ValueError): raise RuntimeError( - "Attempted to remove parking lot breaker for task that is not registered as a breaker", - ) - GLOBAL_PARKING_LOT_BREAKER[task].remove(lot) + "Attempted to remove task as breaker for a lot it is not registered for", + ) from None @attrs.frozen @@ -271,16 +272,18 @@ def break_lot(self, task: Task) -> None: future tasks attempting to park (and unpark? repark?) to error. The error contains a reference to the task sent as a parameter.""" self.broken_by = task - # TODO: weird to phrase this one, we probably should reraise this error in Lock - error = outcome.Error( - _core.BrokenResourceError(f"Parking lot broken by {task}"), - ) # TODO: is there any reason to use self._pop_several? for parked_task in self._parked: if parked_task is task: continue - _core.reschedule(parked_task, error) + # TODO: weird to phrase this one, we maybe should reraise this error in Lock? + _core.reschedule( + parked_task, + outcome.Error( + _core.BrokenResourceError(f"Parking lot broken by {task}"), + ), + ) self._parked.clear() def statistics(self) -> ParkingLotStatistics: From 1f75d441f4cd842409058248c65c966eca61b45a Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 6 Sep 2024 15:55:22 +0200 Subject: [PATCH 06/18] add tests --- src/trio/_core/_parking_lot.py | 2 - src/trio/_core/_tests/test_parking_lot.py | 70 +++++++++++++++++++++++ 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index f515a77291..cf6f02b954 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -275,8 +275,6 @@ def break_lot(self, task: Task) -> None: # TODO: is there any reason to use self._pop_several? for parked_task in self._parked: - if parked_task is task: - continue # TODO: weird to phrase this one, we maybe should reraise this error in Lock? _core.reschedule( parked_task, diff --git a/src/trio/_core/_tests/test_parking_lot.py b/src/trio/_core/_tests/test_parking_lot.py index ed6a17012e..740b0412c1 100644 --- a/src/trio/_core/_tests/test_parking_lot.py +++ b/src/trio/_core/_tests/test_parking_lot.py @@ -4,6 +4,9 @@ import pytest +import trio.lowlevel +from trio.testing import Matcher, RaisesGroup + from ... import _core from ...testing import wait_all_tasks_blocked from .._parking_lot import ParkingLot @@ -215,3 +218,70 @@ async def test_parking_lot_repark_with_count() -> None: "wake 2", ] lot1.unpark_all() + + +async def test_parking_lot_breaker_basic() -> None: + lot = ParkingLot() + task = trio.lowlevel.current_task() + + with pytest.raises( + RuntimeError, + match="Attempted to remove task as breaker for a lot it is not registered for", + ): + trio.lowlevel.remove_parking_lot_breaker(task, lot) + trio.lowlevel.add_parking_lot_breaker(task, lot) + trio.lowlevel.add_parking_lot_breaker(task, lot) + trio.lowlevel.remove_parking_lot_breaker(task, lot) + trio.lowlevel.remove_parking_lot_breaker(task, lot) + + with pytest.raises( + RuntimeError, + match="Attempted to remove task as breaker for a lot it is not registered for", + ): + trio.lowlevel.remove_parking_lot_breaker(task, lot) + + +async def test_parking_lot_breaker() -> None: + async def bad_parker(lot: ParkingLot, scope: _core.CancelScope) -> None: + trio.lowlevel.add_parking_lot_breaker(trio.lowlevel.current_task(), lot) + with scope: + await trio.sleep_forever() + + lot = ParkingLot() + cs = _core.CancelScope() + + # check that parked task errors + with RaisesGroup( + Matcher(_core.BrokenResourceError, match="^Parking lot broken by"), + ): + async with _core.open_nursery() as nursery: + nursery.start_soon(bad_parker, lot, cs) + await wait_all_tasks_blocked() + + nursery.start_soon(lot.park) + await wait_all_tasks_blocked() + + cs.cancel() + + # check that trying to park in brokena lot errors + with pytest.raises(_core.BrokenResourceError): + await lot.park() + + +async def test_parking_lot_weird() -> None: + """break a parking lot, where the breakee is parked. Doing this is weird, but should probably be supported?? + Although the message makes less sense""" + + async def return_me_and_park( + lot: ParkingLot, + *, + task_status: _core.TaskStatus[_core.Task] = trio.TASK_STATUS_IGNORED, + ) -> None: + task_status.started(_core.current_task()) + await lot.park() + + lot = ParkingLot() + with RaisesGroup(Matcher(_core.BrokenResourceError, match="Parking lot broken by")): + async with _core.open_nursery() as nursery: + task = await nursery.start(return_me_and_park, lot) + lot.break_lot(task) From 6835e873870e7b48c00b09c52bcd76a6ab54bd82 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 6 Sep 2024 16:07:40 +0200 Subject: [PATCH 07/18] add lock handover test --- src/trio/_tests/test_sync.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/trio/_tests/test_sync.py b/src/trio/_tests/test_sync.py index 92fd6beec4..0dfa5f79d3 100644 --- a/src/trio/_tests/test_sync.py +++ b/src/trio/_tests/test_sync.py @@ -610,3 +610,28 @@ async def test_lock_multiple_acquire() -> None: async with trio.open_nursery() as nursery: nursery.start_soon(lock.acquire) nursery.start_soon(lock.acquire) + + +async def test_lock_handover() -> None: + lock = trio.Lock() + lock.acquire_nowait() + child_task: Task | None = None + assert _core._parking_lot.GLOBAL_PARKING_LOT_BREAKER[_core.current_task()] == [ + lock._lot, + ] + + async with trio.open_nursery() as nursery: + nursery.start_soon(lock.acquire) + await wait_all_tasks_blocked() + + lock.release() + + assert len(_core._parking_lot.GLOBAL_PARKING_LOT_BREAKER) == 2 + for task, lots in _core._parking_lot.GLOBAL_PARKING_LOT_BREAKER.items(): + if task == _core.current_task(): + assert lots == [] + else: + child_task = task + assert lots == [lock._lot] + + assert lock._lot.broken_by == child_task From 3b86e80072e7b6dcd177003c9ee663eb11d195ff Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 6 Sep 2024 16:22:06 +0200 Subject: [PATCH 08/18] clean up breaker dict --- src/trio/_core/_run.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index dfb2064832..65f367fadd 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1864,6 +1864,7 @@ def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: if task in GLOBAL_PARKING_LOT_BREAKER: for lot in GLOBAL_PARKING_LOT_BREAKER[task]: lot.break_lot(task) + GLOBAL_PARKING_LOT_BREAKER[task] if "task_exited" in self.instruments: self.instruments.call("task_exited", task) From 94ff9a27ca03213426746a97565fe31c08d1226d Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 6 Sep 2024 16:35:51 +0200 Subject: [PATCH 09/18] clean up GLOBAL_PARKING_LOT_BREAKER when task releases or exits --- src/trio/_core/_parking_lot.py | 2 ++ src/trio/_core/_run.py | 2 +- src/trio/_tests/test_sync.py | 25 +++++++++++++++---------- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index cf6f02b954..ebb7f5b726 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -108,6 +108,8 @@ def remove_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: raise RuntimeError( "Attempted to remove task as breaker for a lot it is not registered for", ) from None + if not GLOBAL_PARKING_LOT_BREAKER[task]: + del GLOBAL_PARKING_LOT_BREAKER[task] @attrs.frozen diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 65f367fadd..99a4551696 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1864,7 +1864,7 @@ def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: if task in GLOBAL_PARKING_LOT_BREAKER: for lot in GLOBAL_PARKING_LOT_BREAKER[task]: lot.break_lot(task) - GLOBAL_PARKING_LOT_BREAKER[task] + del GLOBAL_PARKING_LOT_BREAKER[task] if "task_exited" in self.instruments: self.instruments.call("task_exited", task) diff --git a/src/trio/_tests/test_sync.py b/src/trio/_tests/test_sync.py index 0dfa5f79d3..c7238fdf91 100644 --- a/src/trio/_tests/test_sync.py +++ b/src/trio/_tests/test_sync.py @@ -8,6 +8,7 @@ from trio.testing import Matcher, RaisesGroup from .. import _core +from .._core._parking_lot import GLOBAL_PARKING_LOT_BREAKER from .._sync import * from .._timeouts import sleep_forever from ..testing import assert_checkpoints, wait_all_tasks_blocked @@ -594,6 +595,7 @@ async def test_lock_acquire_unowned_lock() -> None: """Test that trying to acquire a lock whose owner has exited raises an error. Partial fix for https://github.com/python-trio/trio/issues/3035 """ + assert not GLOBAL_PARKING_LOT_BREAKER lock = trio.Lock() async with trio.open_nursery() as nursery: nursery.start_soon(lock.acquire) @@ -602,23 +604,29 @@ async def test_lock_acquire_unowned_lock() -> None: match="^Attempted to park in parking lot broken by", ): await lock.acquire() + assert not GLOBAL_PARKING_LOT_BREAKER async def test_lock_multiple_acquire() -> None: + assert not GLOBAL_PARKING_LOT_BREAKER lock = trio.Lock() with RaisesGroup(Matcher(trio.BrokenResourceError, match="Parking lot broken by")): async with trio.open_nursery() as nursery: nursery.start_soon(lock.acquire) nursery.start_soon(lock.acquire) + assert not GLOBAL_PARKING_LOT_BREAKER async def test_lock_handover() -> None: + assert not GLOBAL_PARKING_LOT_BREAKER lock = trio.Lock() lock.acquire_nowait() child_task: Task | None = None - assert _core._parking_lot.GLOBAL_PARKING_LOT_BREAKER[_core.current_task()] == [ - lock._lot, - ] + assert GLOBAL_PARKING_LOT_BREAKER == { + _core.current_task(): [ + lock._lot, + ], + } async with trio.open_nursery() as nursery: nursery.start_soon(lock.acquire) @@ -626,12 +634,9 @@ async def test_lock_handover() -> None: lock.release() - assert len(_core._parking_lot.GLOBAL_PARKING_LOT_BREAKER) == 2 - for task, lots in _core._parking_lot.GLOBAL_PARKING_LOT_BREAKER.items(): - if task == _core.current_task(): - assert lots == [] - else: - child_task = task - assert lots == [lock._lot] + assert len(GLOBAL_PARKING_LOT_BREAKER) == 1 + child_task = next(iter(GLOBAL_PARKING_LOT_BREAKER)) + assert GLOBAL_PARKING_LOT_BREAKER[child_task] == [lock._lot] assert lock._lot.broken_by == child_task + assert not GLOBAL_PARKING_LOT_BREAKER From eb7a451022a6d02cd0d81489c5e742252c294160 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 10 Sep 2024 15:10:57 +0200 Subject: [PATCH 10/18] add newsfragments, add StalledLockError, reraise BrokenResourceError as it in acquire, update docstrings. docs are failing to build locally but idk wth is wrong --- newsfragments/3035.feature.rst | 1 + newsfragments/3081.feature.rst | 1 + src/trio/__init__.py | 1 + src/trio/_core/_parking_lot.py | 7 +++---- src/trio/_sync.py | 27 +++++++++++++++++++++------ src/trio/_tests/test_sync.py | 11 ++++++++--- 6 files changed, 35 insertions(+), 13 deletions(-) create mode 100644 newsfragments/3035.feature.rst create mode 100644 newsfragments/3081.feature.rst diff --git a/newsfragments/3035.feature.rst b/newsfragments/3035.feature.rst new file mode 100644 index 0000000000..bc517e8b88 --- /dev/null +++ b/newsfragments/3035.feature.rst @@ -0,0 +1 @@ +:class:`trio.Lock` and :class:`trio.StrictFIFOLock` will now raise :exc:`trio.StalledLockError` when ``acquire()`` would previously stall due to the owner of the lock having exited without releasing the lock. diff --git a/newsfragments/3081.feature.rst b/newsfragments/3081.feature.rst new file mode 100644 index 0000000000..07fde10abf --- /dev/null +++ b/newsfragments/3081.feature.rst @@ -0,0 +1 @@ +Added :func:`trio.lowlevel.add_parking_lot_breaker` and :func:`trio.lowlevel.remove_parking_lot_breaker` to allow creating custom lock/semaphore implementations that will break their underlying parking lot if a task exits unexpectedly. :meth:`trio.lowlevel.ParkingLot.break_lot` is also added, to allow breaking a parking lot intentionally. Breaking a parking lot raises :exc:`trio.BrokenResourceError` for all tasks currently parked in the lot, and any tasks attempting to park in an already broken parking lot will also error. The breakage status of a lot can be viewed and manually modified with the ``trio.ParkingLot.broken_by`` attribute. diff --git a/src/trio/__init__.py b/src/trio/__init__.py index d2151677b1..b938569a7f 100644 --- a/src/trio/__init__.py +++ b/src/trio/__init__.py @@ -91,6 +91,7 @@ Lock as Lock, LockStatistics as LockStatistics, Semaphore as Semaphore, + StalledLockError as StalledLockError, StrictFIFOLock as StrictFIFOLock, ) from ._timeouts import ( diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index ebb7f5b726..9fea4e5b25 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -269,15 +269,14 @@ def repark_all(self, new_lot: ParkingLot) -> None: """ return self.repark(new_lot, count=len(self)) - def break_lot(self, task: Task) -> None: + def break_lot(self, task: Task | None) -> None: """Break this lot, causing all parked tasks to raise an error, and any future tasks attempting to park (and unpark? repark?) to error. The error - contains a reference to the task sent as a parameter.""" + contains a reference to the task sent as a parameter. + """ self.broken_by = task - # TODO: is there any reason to use self._pop_several? for parked_task in self._parked: - # TODO: weird to phrase this one, we maybe should reraise this error in Lock? _core.reschedule( parked_task, outcome.Error( diff --git a/src/trio/_sync.py b/src/trio/_sync.py index 9a812f68a4..60aa3c970d 100644 --- a/src/trio/_sync.py +++ b/src/trio/_sync.py @@ -19,6 +19,11 @@ from ._core._parking_lot import ParkingLotStatistics +class StalledLockError(Exception): + """Raised by :meth:`Lock.acquire` and :meth:`StrictFIFOLock.acquire` if the owner + exits, or has previously exited, without releasing the lock.""" + + @attrs.frozen class EventStatistics: """An object containing debugging information. @@ -586,16 +591,21 @@ async def acquire(self) -> None: """Acquire the lock, blocking if necessary. Raises: - BrokenResourceError: if the owner of the lock exits without releasing. + StalledLockError: if the owner of the lock exits without releasing. """ await trio.lowlevel.checkpoint_if_cancelled() try: self.acquire_nowait() except trio.WouldBlock: - # NOTE: it's important that the contended acquire path is just - # "_lot.park()", because that's how Condition.wait() acquires the - # lock as well. - await self._lot.park() + try: + # NOTE: it's important that the contended acquire path is just + # "_lot.park()", because that's how Condition.wait() acquires the + # lock as well. + await self._lot.park() + except trio.BrokenResourceError: + raise StalledLockError( + "Owner of this lock exited without releasing: {self._owner}", + ) from None else: await trio.lowlevel.cancel_shielded_checkpoint() @@ -775,7 +785,11 @@ def acquire_nowait(self) -> None: return self._lock.acquire_nowait() async def acquire(self) -> None: - """Acquire the underlying lock, blocking if necessary.""" + """Acquire the underlying lock, blocking if necessary. + + Raises: + StalledLockError: if the owner of the lock exits without releasing. + """ await self._lock.acquire() def release(self) -> None: @@ -804,6 +818,7 @@ async def wait(self) -> None: Raises: RuntimeError: if the calling task does not hold the lock. + StalledLockError: if the owner of the lock exits without releasing, when attempting to re-acquire. """ if trio.lowlevel.current_task() is not self._lock._owner: diff --git a/src/trio/_tests/test_sync.py b/src/trio/_tests/test_sync.py index c7238fdf91..6b4ac97b01 100644 --- a/src/trio/_tests/test_sync.py +++ b/src/trio/_tests/test_sync.py @@ -600,8 +600,8 @@ async def test_lock_acquire_unowned_lock() -> None: async with trio.open_nursery() as nursery: nursery.start_soon(lock.acquire) with pytest.raises( - trio.BrokenResourceError, - match="^Attempted to park in parking lot broken by", + trio.StalledLockError, + match="^Owner of this lock exited without releasing", ): await lock.acquire() assert not GLOBAL_PARKING_LOT_BREAKER @@ -610,7 +610,12 @@ async def test_lock_acquire_unowned_lock() -> None: async def test_lock_multiple_acquire() -> None: assert not GLOBAL_PARKING_LOT_BREAKER lock = trio.Lock() - with RaisesGroup(Matcher(trio.BrokenResourceError, match="Parking lot broken by")): + with RaisesGroup( + Matcher( + trio.StalledLockError, + match="^Owner of this lock exited without releasing", + ), + ): async with trio.open_nursery() as nursery: nursery.start_soon(lock.acquire) nursery.start_soon(lock.acquire) From e7d7205216c59ccc8455358c9c26e0a548c6e803 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 10 Sep 2024 15:14:02 +0200 Subject: [PATCH 11/18] add test for default argument of break_lot --- src/trio/_core/_parking_lot.py | 4 +++- src/trio/_core/_tests/test_parking_lot.py | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index 9fea4e5b25..9dd3b42dd7 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -269,11 +269,13 @@ def repark_all(self, new_lot: ParkingLot) -> None: """ return self.repark(new_lot, count=len(self)) - def break_lot(self, task: Task | None) -> None: + def break_lot(self, task: Task | None = None) -> None: """Break this lot, causing all parked tasks to raise an error, and any future tasks attempting to park (and unpark? repark?) to error. The error contains a reference to the task sent as a parameter. """ + if task is None: + task = _core.current_task() self.broken_by = task for parked_task in self._parked: diff --git a/src/trio/_core/_tests/test_parking_lot.py b/src/trio/_core/_tests/test_parking_lot.py index 740b0412c1..5bfd009126 100644 --- a/src/trio/_core/_tests/test_parking_lot.py +++ b/src/trio/_core/_tests/test_parking_lot.py @@ -240,6 +240,9 @@ async def test_parking_lot_breaker_basic() -> None: ): trio.lowlevel.remove_parking_lot_breaker(task, lot) + lot.break_lot() + assert lot.broken_by == task + async def test_parking_lot_breaker() -> None: async def bad_parker(lot: ParkingLot, scope: _core.CancelScope) -> None: From 277c7da13cd9d18cb1b0b9ab1cc4aa8bbc5a2106 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 18 Sep 2024 12:14:36 +0200 Subject: [PATCH 12/18] various fixes after review --- docs/source/reference-lowlevel.rst | 4 ++++ newsfragments/3035.feature.rst | 2 +- newsfragments/3081.feature.rst | 2 +- src/trio/__init__.py | 1 - src/trio/_core/_parking_lot.py | 23 +++++++++++++++---- src/trio/_core/_tests/test_parking_lot.py | 28 ++++++++++++++++++++++- src/trio/_sync.py | 13 ++++------- src/trio/_tests/test_sync.py | 4 ++-- 8 files changed, 57 insertions(+), 20 deletions(-) diff --git a/docs/source/reference-lowlevel.rst b/docs/source/reference-lowlevel.rst index 70133b9839..10c1ddfdc0 100644 --- a/docs/source/reference-lowlevel.rst +++ b/docs/source/reference-lowlevel.rst @@ -393,6 +393,10 @@ Wait queue abstraction .. autoclass:: ParkingLotStatistics :members: +.. autofunction:: add_parking_lot_breaker + +.. autofunction:: remove_parking_lot_breaker + Low-level checkpoint functions ------------------------------ diff --git a/newsfragments/3035.feature.rst b/newsfragments/3035.feature.rst index bc517e8b88..c25841c47c 100644 --- a/newsfragments/3035.feature.rst +++ b/newsfragments/3035.feature.rst @@ -1 +1 @@ -:class:`trio.Lock` and :class:`trio.StrictFIFOLock` will now raise :exc:`trio.StalledLockError` when ``acquire()`` would previously stall due to the owner of the lock having exited without releasing the lock. +:class:`trio.Lock` and :class:`trio.StrictFIFOLock` will now raise :exc:`trio.BrokenResourceError` when :meth:`trio.Lock.acquire` would previously stall due to the owner of the lock having exited without releasing the lock. diff --git a/newsfragments/3081.feature.rst b/newsfragments/3081.feature.rst index 07fde10abf..34a073b265 100644 --- a/newsfragments/3081.feature.rst +++ b/newsfragments/3081.feature.rst @@ -1 +1 @@ -Added :func:`trio.lowlevel.add_parking_lot_breaker` and :func:`trio.lowlevel.remove_parking_lot_breaker` to allow creating custom lock/semaphore implementations that will break their underlying parking lot if a task exits unexpectedly. :meth:`trio.lowlevel.ParkingLot.break_lot` is also added, to allow breaking a parking lot intentionally. Breaking a parking lot raises :exc:`trio.BrokenResourceError` for all tasks currently parked in the lot, and any tasks attempting to park in an already broken parking lot will also error. The breakage status of a lot can be viewed and manually modified with the ``trio.ParkingLot.broken_by`` attribute. +Added :func:`trio.lowlevel.add_parking_lot_breaker` and :func:`trio.lowlevel.remove_parking_lot_breaker` to allow creating custom lock/semaphore implementations that will break their underlying parking lot if a task exits unexpectedly. :meth:`trio.lowlevel.ParkingLot.break_lot` is also added, to allow breaking a parking lot intentionally. diff --git a/src/trio/__init__.py b/src/trio/__init__.py index b938569a7f..d2151677b1 100644 --- a/src/trio/__init__.py +++ b/src/trio/__init__.py @@ -91,7 +91,6 @@ Lock as Lock, LockStatistics as LockStatistics, Semaphore as Semaphore, - StalledLockError as StalledLockError, StrictFIFOLock as StrictFIFOLock, ) from ._timeouts import ( diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index 9dd3b42dd7..e7be7913e7 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -72,6 +72,7 @@ from __future__ import annotations import math +import warnings from collections import OrderedDict from typing import TYPE_CHECKING @@ -91,9 +92,11 @@ def add_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: - """Register a task as a breaker for a lot. This means that if the task exits without - having unparked from the lot, then the lot will break and raise an error for all tasks - parked in the lot, as well as any future task that attempt to park in it.""" + """Register a task as a breaker for a lot. If this task exits without being removed + as a breaker, the lot will break. This will cause an error to be raised for all + tasks currently parked in the lot, as well as any future tasks that attempt to + park in it. + """ if task not in GLOBAL_PARKING_LOT_BREAKER: GLOBAL_PARKING_LOT_BREAKER[task] = [lot] else: @@ -271,11 +274,21 @@ def repark_all(self, new_lot: ParkingLot) -> None: def break_lot(self, task: Task | None = None) -> None: """Break this lot, causing all parked tasks to raise an error, and any - future tasks attempting to park (and unpark? repark?) to error. The error - contains a reference to the task sent as a parameter. + future tasks attempting to park to error. Unpark & repark become no-ops as the + parking lot is empty. + The error raised contains a reference to the task sent as a parameter. """ if task is None: task = _core.current_task() + if self.broken_by is not None: + if self.broken_by != task: + warnings.warn( + RuntimeWarning( + f"{task} attempted to break parking lot {self} already broken by {self.broken_by}", + ), + stacklevel=2, + ) + return self.broken_by = task for parked_task in self._parked: diff --git a/src/trio/_core/_tests/test_parking_lot.py b/src/trio/_core/_tests/test_parking_lot.py index 5bfd009126..59c4a1a4c2 100644 --- a/src/trio/_core/_tests/test_parking_lot.py +++ b/src/trio/_core/_tests/test_parking_lot.py @@ -229,6 +229,8 @@ async def test_parking_lot_breaker_basic() -> None: match="Attempted to remove task as breaker for a lot it is not registered for", ): trio.lowlevel.remove_parking_lot_breaker(task, lot) + + # check that a task can be registered as breaker for the same lot multiple times trio.lowlevel.add_parking_lot_breaker(task, lot) trio.lowlevel.add_parking_lot_breaker(task, lot) trio.lowlevel.remove_parking_lot_breaker(task, lot) @@ -240,9 +242,33 @@ async def test_parking_lot_breaker_basic() -> None: ): trio.lowlevel.remove_parking_lot_breaker(task, lot) + # defaults to current task lot.break_lot() assert lot.broken_by == task + # breaking the lot again with the same task is a no-op + lot.break_lot() + + # but with a different task it gives a warning + async def dummy_task( + task_status: _core.TaskStatus[_core.Task] = trio.TASK_STATUS_IGNORED, + ) -> None: + task_status.started(_core.current_task()) + + # The nursery is only to create a task we can pass to lot.break_lot + # and has no effect on the test otherwise. + async with trio.open_nursery() as nursery: + child_task = await nursery.start(dummy_task) + with pytest.warns( + RuntimeWarning, + match="attempted to break parking .* already broken by .*", + ): + lot.break_lot(child_task) + nursery.cancel_scope.cancel() + + # and doesn't change broken_by + assert lot.broken_by == task + async def test_parking_lot_breaker() -> None: async def bad_parker(lot: ParkingLot, scope: _core.CancelScope) -> None: @@ -266,7 +292,7 @@ async def bad_parker(lot: ParkingLot, scope: _core.CancelScope) -> None: cs.cancel() - # check that trying to park in brokena lot errors + # check that trying to park in broken lot errors with pytest.raises(_core.BrokenResourceError): await lot.park() diff --git a/src/trio/_sync.py b/src/trio/_sync.py index 60aa3c970d..3d7d28914b 100644 --- a/src/trio/_sync.py +++ b/src/trio/_sync.py @@ -19,11 +19,6 @@ from ._core._parking_lot import ParkingLotStatistics -class StalledLockError(Exception): - """Raised by :meth:`Lock.acquire` and :meth:`StrictFIFOLock.acquire` if the owner - exits, or has previously exited, without releasing the lock.""" - - @attrs.frozen class EventStatistics: """An object containing debugging information. @@ -591,7 +586,7 @@ async def acquire(self) -> None: """Acquire the lock, blocking if necessary. Raises: - StalledLockError: if the owner of the lock exits without releasing. + BrokenResourceError: if the owner of the lock exits without releasing. """ await trio.lowlevel.checkpoint_if_cancelled() try: @@ -603,7 +598,7 @@ async def acquire(self) -> None: # lock as well. await self._lot.park() except trio.BrokenResourceError: - raise StalledLockError( + raise trio.BrokenResourceError( "Owner of this lock exited without releasing: {self._owner}", ) from None else: @@ -788,7 +783,7 @@ async def acquire(self) -> None: """Acquire the underlying lock, blocking if necessary. Raises: - StalledLockError: if the owner of the lock exits without releasing. + BrokenResourceError: if the owner of the lock exits without releasing. """ await self._lock.acquire() @@ -818,7 +813,7 @@ async def wait(self) -> None: Raises: RuntimeError: if the calling task does not hold the lock. - StalledLockError: if the owner of the lock exits without releasing, when attempting to re-acquire. + BrokenResourceError: if the owner of the lock exits without releasing, when attempting to re-acquire. """ if trio.lowlevel.current_task() is not self._lock._owner: diff --git a/src/trio/_tests/test_sync.py b/src/trio/_tests/test_sync.py index 6b4ac97b01..401ab0f55f 100644 --- a/src/trio/_tests/test_sync.py +++ b/src/trio/_tests/test_sync.py @@ -600,7 +600,7 @@ async def test_lock_acquire_unowned_lock() -> None: async with trio.open_nursery() as nursery: nursery.start_soon(lock.acquire) with pytest.raises( - trio.StalledLockError, + trio.BrokenResourceError, match="^Owner of this lock exited without releasing", ): await lock.acquire() @@ -612,7 +612,7 @@ async def test_lock_multiple_acquire() -> None: lock = trio.Lock() with RaisesGroup( Matcher( - trio.StalledLockError, + trio.BrokenResourceError, match="^Owner of this lock exited without releasing", ), ): From 45f78f46e636e6672179292fe69f5f39fb87e2d9 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 19 Sep 2024 12:30:41 +0200 Subject: [PATCH 13/18] break lots before other checks, minor phrasing improvement in docstring --- src/trio/_core/_run.py | 11 +++++------ src/trio/_sync.py | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 99a4551696..3ea7c269b3 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1821,6 +1821,11 @@ async def python_wrapper(orig_coro: Awaitable[RetT]) -> RetT: return task def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: + if task in GLOBAL_PARKING_LOT_BREAKER: + for lot in GLOBAL_PARKING_LOT_BREAKER[task]: + lot.break_lot(task) + del GLOBAL_PARKING_LOT_BREAKER[task] + if ( task._cancel_status is not None and task._cancel_status.abandoned_by_misnesting @@ -1860,12 +1865,6 @@ def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: assert task._parent_nursery is not None, task task._parent_nursery._child_finished(task, outcome) - # before or after the other stuff in this function? - if task in GLOBAL_PARKING_LOT_BREAKER: - for lot in GLOBAL_PARKING_LOT_BREAKER[task]: - lot.break_lot(task) - del GLOBAL_PARKING_LOT_BREAKER[task] - if "task_exited" in self.instruments: self.instruments.call("task_exited", task) diff --git a/src/trio/_sync.py b/src/trio/_sync.py index 3d7d28914b..1b72d7ec76 100644 --- a/src/trio/_sync.py +++ b/src/trio/_sync.py @@ -783,7 +783,7 @@ async def acquire(self) -> None: """Acquire the underlying lock, blocking if necessary. Raises: - BrokenResourceError: if the owner of the lock exits without releasing. + BrokenResourceError: if the owner of the underlying lock exits without releasing. """ await self._lock.acquire() From ec488633c597f84c4b0c952a82bbb80aa0222097 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Fri, 27 Sep 2024 12:57:26 +0200 Subject: [PATCH 14/18] docstring updates after A5rocks review --- src/trio/_core/_parking_lot.py | 16 ++++++++-------- src/trio/_core/_run.py | 1 + src/trio/_core/_tests/test_parking_lot.py | 4 +++- src/trio/_sync.py | 12 +++++++++--- src/trio/_tests/test_sync.py | 4 +++- 5 files changed, 24 insertions(+), 13 deletions(-) diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index e7be7913e7..eaf9225c2c 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -92,11 +92,7 @@ def add_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: - """Register a task as a breaker for a lot. If this task exits without being removed - as a breaker, the lot will break. This will cause an error to be raised for all - tasks currently parked in the lot, as well as any future tasks that attempt to - park in it. - """ + """Register a task as a breaker for a lot. See :meth:`ParkingLot.break_lot`""" if task not in GLOBAL_PARKING_LOT_BREAKER: GLOBAL_PARKING_LOT_BREAKER[task] = [lot] else: @@ -104,7 +100,7 @@ def add_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: def remove_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: - """Deregister a task as a breaker for a lot. See :func:`add_parking_lot_breaker`.""" + """Deregister a task as a breaker for a lot. See :meth:`ParkingLot.break_lot`""" try: GLOBAL_PARKING_LOT_BREAKER[task].remove(lot) except (KeyError, ValueError): @@ -273,10 +269,14 @@ def repark_all(self, new_lot: ParkingLot) -> None: return self.repark(new_lot, count=len(self)) def break_lot(self, task: Task | None = None) -> None: - """Break this lot, causing all parked tasks to raise an error, and any + """Break this lot, with ``task`` noted as the task that broke it. + + This causes all parked tasks to raise an error, and any future tasks attempting to park to error. Unpark & repark become no-ops as the parking lot is empty. - The error raised contains a reference to the task sent as a parameter. + + The error raised contains a reference to the task sent as a parameter. It is also + saved in the ``broken_by`` attribute. """ if task is None: task = _core.current_task() diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 3ea7c269b3..4fa651313e 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1821,6 +1821,7 @@ async def python_wrapper(orig_coro: Awaitable[RetT]) -> RetT: return task def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: + # break parking lots associated with the task exiting if task in GLOBAL_PARKING_LOT_BREAKER: for lot in GLOBAL_PARKING_LOT_BREAKER[task]: lot.break_lot(task) diff --git a/src/trio/_core/_tests/test_parking_lot.py b/src/trio/_core/_tests/test_parking_lot.py index 59c4a1a4c2..8cfaf84b24 100644 --- a/src/trio/_core/_tests/test_parking_lot.py +++ b/src/trio/_core/_tests/test_parking_lot.py @@ -310,7 +310,9 @@ async def return_me_and_park( await lot.park() lot = ParkingLot() - with RaisesGroup(Matcher(_core.BrokenResourceError, match="Parking lot broken by")): + with RaisesGroup( + Matcher(_core.BrokenResourceError, match="^Parking lot broken by"), + ): async with _core.open_nursery() as nursery: task = await nursery.start(return_me_and_park, lot) lot.break_lot(task) diff --git a/src/trio/_sync.py b/src/trio/_sync.py index 1b72d7ec76..03d518aab9 100644 --- a/src/trio/_sync.py +++ b/src/trio/_sync.py @@ -8,8 +8,14 @@ import trio from . import _core -from ._core import Abort, ParkingLot, RaiseCancelT, enable_ki_protection -from ._core._parking_lot import add_parking_lot_breaker, remove_parking_lot_breaker +from ._core import ( + Abort, + ParkingLot, + RaiseCancelT, + add_parking_lot_breaker, + enable_ki_protection, + remove_parking_lot_breaker, +) from ._util import final if TYPE_CHECKING: @@ -599,7 +605,7 @@ async def acquire(self) -> None: await self._lot.park() except trio.BrokenResourceError: raise trio.BrokenResourceError( - "Owner of this lock exited without releasing: {self._owner}", + f"Owner of this lock exited without releasing: {self._owner}", ) from None else: await trio.lowlevel.cancel_shielded_checkpoint() diff --git a/src/trio/_tests/test_sync.py b/src/trio/_tests/test_sync.py index 401ab0f55f..1a0b230e3e 100644 --- a/src/trio/_tests/test_sync.py +++ b/src/trio/_tests/test_sync.py @@ -593,7 +593,7 @@ async def lock_taker() -> None: async def test_lock_acquire_unowned_lock() -> None: """Test that trying to acquire a lock whose owner has exited raises an error. - Partial fix for https://github.com/python-trio/trio/issues/3035 + see https://github.com/python-trio/trio/issues/3035 """ assert not GLOBAL_PARKING_LOT_BREAKER lock = trio.Lock() @@ -608,6 +608,8 @@ async def test_lock_acquire_unowned_lock() -> None: async def test_lock_multiple_acquire() -> None: + """Test for error if awaiting on a lock whose owner exits without releasing. + see https://github.com/python-trio/trio/issues/3035""" assert not GLOBAL_PARKING_LOT_BREAKER lock = trio.Lock() with RaisesGroup( From 7a1ce5b755b5775153a3525bea793ab7da05fb44 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 2 Oct 2024 12:41:47 +0200 Subject: [PATCH 15/18] raise brokenresourceerror if registering an already exited task. fix docstring. fix runtimewarning transforming into triointernalerror. add a bunch of tests --- src/trio/_core/_parking_lot.py | 15 +++- src/trio/_core/_run.py | 2 + src/trio/_core/_tests/test_parking_lot.py | 100 ++++++++++++++++++---- 3 files changed, 96 insertions(+), 21 deletions(-) diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index eaf9225c2c..3f2edc5bac 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -71,6 +71,7 @@ # See: https://github.com/python-trio/trio/issues/53 from __future__ import annotations +import inspect import math import warnings from collections import OrderedDict @@ -92,7 +93,15 @@ def add_parking_lot_breaker(task: Task, lot: ParkingLot) -> None: - """Register a task as a breaker for a lot. See :meth:`ParkingLot.break_lot`""" + """Register a task as a breaker for a lot. See :meth:`ParkingLot.break_lot`. + + raises: + trio.BrokenResourceError: if the task has already exited. + """ + if inspect.getcoroutinestate(task.coro) == inspect.CORO_CLOSED: + raise _core._exceptions.BrokenResourceError( + "Attempted to add already exited task as lot breaker.", + ) if task not in GLOBAL_PARKING_LOT_BREAKER: GLOBAL_PARKING_LOT_BREAKER[task] = [lot] else: @@ -275,8 +284,8 @@ def break_lot(self, task: Task | None = None) -> None: future tasks attempting to park to error. Unpark & repark become no-ops as the parking lot is empty. - The error raised contains a reference to the task sent as a parameter. It is also - saved in the ``broken_by`` attribute. + The error raised contains a reference to the task sent as a parameter. The task + is also saved in the parking lot in the ``broken_by`` attribute. """ if task is None: task = _core.current_task() diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index defbaa6a80..962a649cd3 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -2800,6 +2800,8 @@ def unrolled_run( ), stacklevel=1, ) + except RuntimeWarning: + raise except TrioInternalError: raise except BaseException as exc: diff --git a/src/trio/_core/_tests/test_parking_lot.py b/src/trio/_core/_tests/test_parking_lot.py index 8cfaf84b24..9ad4ac693b 100644 --- a/src/trio/_core/_tests/test_parking_lot.py +++ b/src/trio/_core/_tests/test_parking_lot.py @@ -4,7 +4,12 @@ import pytest -import trio.lowlevel +import trio +from trio.lowlevel import ( + add_parking_lot_breaker, + current_task, + remove_parking_lot_breaker, +) from trio.testing import Matcher, RaisesGroup from ... import _core @@ -220,27 +225,34 @@ async def test_parking_lot_repark_with_count() -> None: lot1.unpark_all() +async def dummy_task( + task_status: _core.TaskStatus[_core.Task] = trio.TASK_STATUS_IGNORED, +) -> None: + task_status.started(_core.current_task()) + await trio.sleep_forever() + + async def test_parking_lot_breaker_basic() -> None: lot = ParkingLot() - task = trio.lowlevel.current_task() + task = current_task() with pytest.raises( RuntimeError, match="Attempted to remove task as breaker for a lot it is not registered for", ): - trio.lowlevel.remove_parking_lot_breaker(task, lot) + remove_parking_lot_breaker(task, lot) # check that a task can be registered as breaker for the same lot multiple times - trio.lowlevel.add_parking_lot_breaker(task, lot) - trio.lowlevel.add_parking_lot_breaker(task, lot) - trio.lowlevel.remove_parking_lot_breaker(task, lot) - trio.lowlevel.remove_parking_lot_breaker(task, lot) + add_parking_lot_breaker(task, lot) + add_parking_lot_breaker(task, lot) + remove_parking_lot_breaker(task, lot) + remove_parking_lot_breaker(task, lot) with pytest.raises( RuntimeError, match="Attempted to remove task as breaker for a lot it is not registered for", ): - trio.lowlevel.remove_parking_lot_breaker(task, lot) + remove_parking_lot_breaker(task, lot) # defaults to current task lot.break_lot() @@ -249,30 +261,81 @@ async def test_parking_lot_breaker_basic() -> None: # breaking the lot again with the same task is a no-op lot.break_lot() - # but with a different task it gives a warning - async def dummy_task( - task_status: _core.TaskStatus[_core.Task] = trio.TASK_STATUS_IGNORED, - ) -> None: - task_status.started(_core.current_task()) + child_task = None + with pytest.warns(RuntimeWarning): + async with trio.open_nursery() as nursery: + child_task = await nursery.start(dummy_task) + # registering a task as breaker on an already broken lot is fine... though it + # maybe shouldn't be as it will always cause a RuntimeWarning??? + # Or is this a sign that we shouldn't raise a warning? + add_parking_lot_breaker(child_task, lot) + nursery.cancel_scope.cancel() + + # manually breaking a lot with an already exited task is fine + lot = ParkingLot() + lot.break_lot(child_task) + + +async def test_parking_lot_breaker_warnings() -> None: + lot = ParkingLot() + task = current_task() + lot.break_lot() + warn_str = "attempted to break parking .* already broken by .*" + # breaking an already broken lot with a different task gives a warning # The nursery is only to create a task we can pass to lot.break_lot - # and has no effect on the test otherwise. async with trio.open_nursery() as nursery: child_task = await nursery.start(dummy_task) with pytest.warns( RuntimeWarning, - match="attempted to break parking .* already broken by .*", + match=warn_str, ): lot.break_lot(child_task) nursery.cancel_scope.cancel() + # note that this get put into an exceptiongroup if inside a nursery, making any + # stacklevel arguments irrelevant + with RaisesGroup(Matcher(RuntimeWarning, match=warn_str)): + async with trio.open_nursery() as nursery: + child_task = await nursery.start(dummy_task) + lot.break_lot(child_task) + nursery.cancel_scope.cancel() + # and doesn't change broken_by assert lot.broken_by == task + # register multiple tasks as lot breakers, then have them all exit + lot = ParkingLot() + child_task = None + # This does not give an exception group, as the warning is raised by the nursery + # exiting, and not any of the tasks inside the nursery. + # And we only get a single warning because... of the default warning filter? and the + # location being the same? (because I haven't figured out why stacklevel makes no + # difference) + with pytest.warns( + RuntimeWarning, + match=warn_str, + ): + async with trio.open_nursery() as nursery: + child_task = await nursery.start(dummy_task) + child_task2 = await nursery.start(dummy_task) + child_task3 = await nursery.start(dummy_task) + add_parking_lot_breaker(child_task, lot) + add_parking_lot_breaker(child_task2, lot) + add_parking_lot_breaker(child_task3, lot) + nursery.cancel_scope.cancel() + + # trying to register an exited task as lot breaker errors + with pytest.raises( + trio.BrokenResourceError, + match="^Attempted to add already exited task as lot breaker.$", + ): + add_parking_lot_breaker(child_task, lot) + async def test_parking_lot_breaker() -> None: async def bad_parker(lot: ParkingLot, scope: _core.CancelScope) -> None: - trio.lowlevel.add_parking_lot_breaker(trio.lowlevel.current_task(), lot) + add_parking_lot_breaker(current_task(), lot) with scope: await trio.sleep_forever() @@ -298,8 +361,9 @@ async def bad_parker(lot: ParkingLot, scope: _core.CancelScope) -> None: async def test_parking_lot_weird() -> None: - """break a parking lot, where the breakee is parked. Doing this is weird, but should probably be supported?? - Although the message makes less sense""" + """Break a parking lot, where the breakee is parked. + Doing this is weird, but should probably be supported. + """ async def return_me_and_park( lot: ParkingLot, From cc97cca24fc52f73d18581cbf6bd77fceeb3dd6f Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 7 Oct 2024 14:52:06 +0200 Subject: [PATCH 16/18] remove warning on task exit --- src/trio/_core/_run.py | 5 +-- src/trio/_core/_tests/test_parking_lot.py | 49 +++++++---------------- 2 files changed, 17 insertions(+), 37 deletions(-) diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 962a649cd3..871016963c 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1900,7 +1900,8 @@ def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: # break parking lots associated with the task exiting if task in GLOBAL_PARKING_LOT_BREAKER: for lot in GLOBAL_PARKING_LOT_BREAKER[task]: - lot.break_lot(task) + if lot.broken_by is None: + lot.break_lot(task) del GLOBAL_PARKING_LOT_BREAKER[task] if ( @@ -2800,8 +2801,6 @@ def unrolled_run( ), stacklevel=1, ) - except RuntimeWarning: - raise except TrioInternalError: raise except BaseException as exc: diff --git a/src/trio/_core/_tests/test_parking_lot.py b/src/trio/_core/_tests/test_parking_lot.py index 9ad4ac693b..b8cb97712f 100644 --- a/src/trio/_core/_tests/test_parking_lot.py +++ b/src/trio/_core/_tests/test_parking_lot.py @@ -261,15 +261,12 @@ async def test_parking_lot_breaker_basic() -> None: # breaking the lot again with the same task is a no-op lot.break_lot() + # registering a task as a breaker on an already broken lot is a no-op. child_task = None - with pytest.warns(RuntimeWarning): - async with trio.open_nursery() as nursery: - child_task = await nursery.start(dummy_task) - # registering a task as breaker on an already broken lot is fine... though it - # maybe shouldn't be as it will always cause a RuntimeWarning??? - # Or is this a sign that we shouldn't raise a warning? - add_parking_lot_breaker(child_task, lot) - nursery.cancel_scope.cancel() + async with trio.open_nursery() as nursery: + child_task = await nursery.start(dummy_task) + add_parking_lot_breaker(child_task, lot) + nursery.cancel_scope.cancel() # manually breaking a lot with an already exited task is fine lot = ParkingLot() @@ -293,37 +290,21 @@ async def test_parking_lot_breaker_warnings() -> None: lot.break_lot(child_task) nursery.cancel_scope.cancel() - # note that this get put into an exceptiongroup if inside a nursery, making any - # stacklevel arguments irrelevant - with RaisesGroup(Matcher(RuntimeWarning, match=warn_str)): - async with trio.open_nursery() as nursery: - child_task = await nursery.start(dummy_task) - lot.break_lot(child_task) - nursery.cancel_scope.cancel() - # and doesn't change broken_by assert lot.broken_by == task # register multiple tasks as lot breakers, then have them all exit + # No warning is given on task exit, even if the lot is already broken. lot = ParkingLot() child_task = None - # This does not give an exception group, as the warning is raised by the nursery - # exiting, and not any of the tasks inside the nursery. - # And we only get a single warning because... of the default warning filter? and the - # location being the same? (because I haven't figured out why stacklevel makes no - # difference) - with pytest.warns( - RuntimeWarning, - match=warn_str, - ): - async with trio.open_nursery() as nursery: - child_task = await nursery.start(dummy_task) - child_task2 = await nursery.start(dummy_task) - child_task3 = await nursery.start(dummy_task) - add_parking_lot_breaker(child_task, lot) - add_parking_lot_breaker(child_task2, lot) - add_parking_lot_breaker(child_task3, lot) - nursery.cancel_scope.cancel() + async with trio.open_nursery() as nursery: + child_task = await nursery.start(dummy_task) + child_task2 = await nursery.start(dummy_task) + child_task3 = await nursery.start(dummy_task) + add_parking_lot_breaker(child_task, lot) + add_parking_lot_breaker(child_task2, lot) + add_parking_lot_breaker(child_task3, lot) + nursery.cancel_scope.cancel() # trying to register an exited task as lot breaker errors with pytest.raises( @@ -333,7 +314,7 @@ async def test_parking_lot_breaker_warnings() -> None: add_parking_lot_breaker(child_task, lot) -async def test_parking_lot_breaker() -> None: +async def test_parking_lot_breaker_bad_parker() -> None: async def bad_parker(lot: ParkingLot, scope: _core.CancelScope) -> None: add_parking_lot_breaker(current_task(), lot) with scope: From 1d7ece30766c7076bdbac84286870d958769a8ab Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 8 Oct 2024 12:37:15 +0200 Subject: [PATCH 17/18] make broken_by attribute a list, clean up tests --- src/trio/_core/_parking_lot.py | 20 ++-- src/trio/_core/_run.py | 3 +- src/trio/_core/_tests/test_parking_lot.py | 123 +++++++++++++--------- 3 files changed, 82 insertions(+), 64 deletions(-) diff --git a/src/trio/_core/_parking_lot.py b/src/trio/_core/_parking_lot.py index 3f2edc5bac..af6ff610ee 100644 --- a/src/trio/_core/_parking_lot.py +++ b/src/trio/_core/_parking_lot.py @@ -73,7 +73,6 @@ import inspect import math -import warnings from collections import OrderedDict from typing import TYPE_CHECKING @@ -152,7 +151,7 @@ class ParkingLot: # {task: None}, we just want a deque where we can quickly delete random # items _parked: OrderedDict[Task, None] = attrs.field(factory=OrderedDict, init=False) - broken_by: Task | None = None + broken_by: list[Task] = attrs.field(factory=list, init=False) def __len__(self) -> int: """Returns the number of parked tasks.""" @@ -176,7 +175,7 @@ async def park(self) -> None: breaks before we get to unpark. """ - if self.broken_by is not None: + if self.broken_by: raise _core.BrokenResourceError( f"Attempted to park in parking lot broken by {self.broken_by}", ) @@ -289,16 +288,13 @@ def break_lot(self, task: Task | None = None) -> None: """ if task is None: task = _core.current_task() - if self.broken_by is not None: - if self.broken_by != task: - warnings.warn( - RuntimeWarning( - f"{task} attempted to break parking lot {self} already broken by {self.broken_by}", - ), - stacklevel=2, - ) + + # if lot is already broken, just mark this as another breaker and return + if self.broken_by: + self.broken_by.append(task) return - self.broken_by = task + + self.broken_by.append(task) for parked_task in self._parked: _core.reschedule( diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index 871016963c..defbaa6a80 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1900,8 +1900,7 @@ def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: # break parking lots associated with the task exiting if task in GLOBAL_PARKING_LOT_BREAKER: for lot in GLOBAL_PARKING_LOT_BREAKER[task]: - if lot.broken_by is None: - lot.break_lot(task) + lot.break_lot(task) del GLOBAL_PARKING_LOT_BREAKER[task] if ( diff --git a/src/trio/_core/_tests/test_parking_lot.py b/src/trio/_core/_tests/test_parking_lot.py index b8cb97712f..cad3e7b431 100644 --- a/src/trio/_core/_tests/test_parking_lot.py +++ b/src/trio/_core/_tests/test_parking_lot.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re from typing import TypeVar import pytest @@ -233,6 +234,53 @@ async def dummy_task( async def test_parking_lot_breaker_basic() -> None: + """Test basic functionality for breaking lots.""" + lot = ParkingLot() + task = current_task() + + # defaults to current task + lot.break_lot() + assert lot.broken_by == [task] + + # breaking the lot again with the same task appends another copy in `broken_by` + lot.break_lot() + assert lot.broken_by == [task, task] + + # trying to park in broken lot errors + broken_by_str = re.escape(str([task, task])) + with pytest.raises( + _core.BrokenResourceError, + match=f"^Attempted to park in parking lot broken by {broken_by_str}$", + ): + await lot.park() + + +async def test_parking_lot_break_parking_tasks() -> None: + """Checks that tasks currently waiting to park raise an error when the breaker exits.""" + + async def bad_parker(lot: ParkingLot, scope: _core.CancelScope) -> None: + add_parking_lot_breaker(current_task(), lot) + with scope: + await trio.sleep_forever() + + lot = ParkingLot() + cs = _core.CancelScope() + + # check that parked task errors + with RaisesGroup( + Matcher(_core.BrokenResourceError, match="^Parking lot broken by"), + ): + async with _core.open_nursery() as nursery: + nursery.start_soon(bad_parker, lot, cs) + await wait_all_tasks_blocked() + + nursery.start_soon(lot.park) + await wait_all_tasks_blocked() + + cs.cancel() + + +async def test_parking_lot_breaker_registration() -> None: lot = ParkingLot() task = current_task() @@ -254,58 +302,60 @@ async def test_parking_lot_breaker_basic() -> None: ): remove_parking_lot_breaker(task, lot) - # defaults to current task - lot.break_lot() - assert lot.broken_by == task - - # breaking the lot again with the same task is a no-op + # registering a task as breaker on an already broken lot is fine lot.break_lot() - - # registering a task as a breaker on an already broken lot is a no-op. child_task = None async with trio.open_nursery() as nursery: child_task = await nursery.start(dummy_task) add_parking_lot_breaker(child_task, lot) nursery.cancel_scope.cancel() + assert lot.broken_by == [task, child_task] # manually breaking a lot with an already exited task is fine lot = ParkingLot() lot.break_lot(child_task) + assert lot.broken_by == [child_task] -async def test_parking_lot_breaker_warnings() -> None: +async def test_parking_lot_breaker_rebreak() -> None: lot = ParkingLot() task = current_task() lot.break_lot() - warn_str = "attempted to break parking .* already broken by .*" - # breaking an already broken lot with a different task gives a warning + # breaking an already broken lot with a different task is allowed # The nursery is only to create a task we can pass to lot.break_lot async with trio.open_nursery() as nursery: child_task = await nursery.start(dummy_task) - with pytest.warns( - RuntimeWarning, - match=warn_str, - ): - lot.break_lot(child_task) + lot.break_lot(child_task) nursery.cancel_scope.cancel() - # and doesn't change broken_by - assert lot.broken_by == task + # and appends the task + assert lot.broken_by == [task, child_task] + +async def test_parking_lot_multiple_breakers_exit() -> None: # register multiple tasks as lot breakers, then have them all exit # No warning is given on task exit, even if the lot is already broken. lot = ParkingLot() - child_task = None async with trio.open_nursery() as nursery: - child_task = await nursery.start(dummy_task) + child_task1 = await nursery.start(dummy_task) child_task2 = await nursery.start(dummy_task) child_task3 = await nursery.start(dummy_task) - add_parking_lot_breaker(child_task, lot) + add_parking_lot_breaker(child_task1, lot) add_parking_lot_breaker(child_task2, lot) add_parking_lot_breaker(child_task3, lot) nursery.cancel_scope.cancel() + # I think the order is guaranteed currently, but doesn't hurt to be safe. + assert set(lot.broken_by) == {child_task1, child_task2, child_task3} + + +async def test_parking_lot_breaker_register_exited_task() -> None: + lot = ParkingLot() + child_task = None + async with trio.open_nursery() as nursery: + child_task = await nursery.start(dummy_task) + nursery.cancel_scope.cancel() # trying to register an exited task as lot breaker errors with pytest.raises( trio.BrokenResourceError, @@ -314,34 +364,7 @@ async def test_parking_lot_breaker_warnings() -> None: add_parking_lot_breaker(child_task, lot) -async def test_parking_lot_breaker_bad_parker() -> None: - async def bad_parker(lot: ParkingLot, scope: _core.CancelScope) -> None: - add_parking_lot_breaker(current_task(), lot) - with scope: - await trio.sleep_forever() - - lot = ParkingLot() - cs = _core.CancelScope() - - # check that parked task errors - with RaisesGroup( - Matcher(_core.BrokenResourceError, match="^Parking lot broken by"), - ): - async with _core.open_nursery() as nursery: - nursery.start_soon(bad_parker, lot, cs) - await wait_all_tasks_blocked() - - nursery.start_soon(lot.park) - await wait_all_tasks_blocked() - - cs.cancel() - - # check that trying to park in broken lot errors - with pytest.raises(_core.BrokenResourceError): - await lot.park() - - -async def test_parking_lot_weird() -> None: +async def test_parking_lot_break_itself() -> None: """Break a parking lot, where the breakee is parked. Doing this is weird, but should probably be supported. """ @@ -359,5 +382,5 @@ async def return_me_and_park( Matcher(_core.BrokenResourceError, match="^Parking lot broken by"), ): async with _core.open_nursery() as nursery: - task = await nursery.start(return_me_and_park, lot) - lot.break_lot(task) + child_task = await nursery.start(return_me_and_park, lot) + lot.break_lot(child_task) From 92f9799d80d25bbf00f5021f41384bf320bb1c5e Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 8 Oct 2024 13:03:47 +0200 Subject: [PATCH 18/18] fix test. polish comments and tests --- newsfragments/3035.feature.rst | 2 +- src/trio/_core/_run.py | 2 +- src/trio/_core/_tests/test_parking_lot.py | 2 -- src/trio/_tests/test_sync.py | 14 ++++++++++---- 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/newsfragments/3035.feature.rst b/newsfragments/3035.feature.rst index c25841c47c..a1761fa282 100644 --- a/newsfragments/3035.feature.rst +++ b/newsfragments/3035.feature.rst @@ -1 +1 @@ -:class:`trio.Lock` and :class:`trio.StrictFIFOLock` will now raise :exc:`trio.BrokenResourceError` when :meth:`trio.Lock.acquire` would previously stall due to the owner of the lock having exited without releasing the lock. +:class:`trio.Lock` and :class:`trio.StrictFIFOLock` will now raise :exc:`trio.BrokenResourceError` when :meth:`trio.Lock.acquire` would previously stall due to the owner of the lock exiting without releasing the lock. diff --git a/src/trio/_core/_run.py b/src/trio/_core/_run.py index defbaa6a80..cba7a8dec0 100644 --- a/src/trio/_core/_run.py +++ b/src/trio/_core/_run.py @@ -1897,7 +1897,7 @@ async def python_wrapper(orig_coro: Awaitable[RetT]) -> RetT: return task def task_exited(self, task: Task, outcome: Outcome[Any]) -> None: - # break parking lots associated with the task exiting + # break parking lots associated with the exiting task if task in GLOBAL_PARKING_LOT_BREAKER: for lot in GLOBAL_PARKING_LOT_BREAKER[task]: lot.break_lot(task) diff --git a/src/trio/_core/_tests/test_parking_lot.py b/src/trio/_core/_tests/test_parking_lot.py index cad3e7b431..d9afee83d4 100644 --- a/src/trio/_core/_tests/test_parking_lot.py +++ b/src/trio/_core/_tests/test_parking_lot.py @@ -329,13 +329,11 @@ async def test_parking_lot_breaker_rebreak() -> None: lot.break_lot(child_task) nursery.cancel_scope.cancel() - # and appends the task assert lot.broken_by == [task, child_task] async def test_parking_lot_multiple_breakers_exit() -> None: # register multiple tasks as lot breakers, then have them all exit - # No warning is given on task exit, even if the lot is already broken. lot = ParkingLot() async with trio.open_nursery() as nursery: child_task1 = await nursery.start(dummy_task) diff --git a/src/trio/_tests/test_sync.py b/src/trio/_tests/test_sync.py index 1a0b230e3e..f506e84ffc 100644 --- a/src/trio/_tests/test_sync.py +++ b/src/trio/_tests/test_sync.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re import weakref from typing import TYPE_CHECKING, Callable, Union @@ -599,9 +600,10 @@ async def test_lock_acquire_unowned_lock() -> None: lock = trio.Lock() async with trio.open_nursery() as nursery: nursery.start_soon(lock.acquire) + owner_str = re.escape(str(lock._lot.broken_by[0])) with pytest.raises( trio.BrokenResourceError, - match="^Owner of this lock exited without releasing", + match=f"^Owner of this lock exited without releasing: {owner_str}$", ): await lock.acquire() assert not GLOBAL_PARKING_LOT_BREAKER @@ -615,7 +617,7 @@ async def test_lock_multiple_acquire() -> None: with RaisesGroup( Matcher( trio.BrokenResourceError, - match="^Owner of this lock exited without releasing", + match="^Owner of this lock exited without releasing: ", ), ): async with trio.open_nursery() as nursery: @@ -626,9 +628,11 @@ async def test_lock_multiple_acquire() -> None: async def test_lock_handover() -> None: assert not GLOBAL_PARKING_LOT_BREAKER + child_task: Task | None = None lock = trio.Lock() + + # this task acquires the lock lock.acquire_nowait() - child_task: Task | None = None assert GLOBAL_PARKING_LOT_BREAKER == { _core.current_task(): [ lock._lot, @@ -639,11 +643,13 @@ async def test_lock_handover() -> None: nursery.start_soon(lock.acquire) await wait_all_tasks_blocked() + # hand over the lock to the child task lock.release() + # check values, and get the identifier out of the dict for later check assert len(GLOBAL_PARKING_LOT_BREAKER) == 1 child_task = next(iter(GLOBAL_PARKING_LOT_BREAKER)) assert GLOBAL_PARKING_LOT_BREAKER[child_task] == [lock._lot] - assert lock._lot.broken_by == child_task + assert lock._lot.broken_by == [child_task] assert not GLOBAL_PARKING_LOT_BREAKER