Add ability to break parking lots, stop locks from stalling #3081

jakkdl · 2024-09-06T10:38:26Z

fixes #3035

TODO:

newsfragment(s)
docstrings
docs
tests of directly using the parking lot functions
lock test that checks that handing over the owner works (this is indirectly tested by a lot of other tests, but good to have a standalone test)
reraise the exception in Lock/_LockImpl to get a better error message

codecov · 2024-09-06T10:43:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@b834f73). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3081   +/-   ##
=======================================
  Coverage        ?   99.60%           
=======================================
  Files           ?      121           
  Lines           ?    18004           
  Branches        ?     3238           
=======================================
  Hits            ?    17933           
  Misses          ?       50           
  Partials        ?       21

Files with missing lines	Coverage Δ
src/trio/_core/__init__.py	`100.00% <100.00%> (ø)`
src/trio/_core/_parking_lot.py	`100.00% <100.00%> (ø)`
src/trio/_core/_run.py	`99.38% <100.00%> (ø)`
src/trio/_core/_tests/test_parking_lot.py	`100.00% <100.00%> (ø)`
src/trio/_sync.py	`100.00% <100.00%> (ø)`
src/trio/_tests/test_sync.py	`100.00% <100.00%> (ø)`
src/trio/lowlevel.py	`100.00% <ø> (ø)`

src/trio/_core/_parking_lot.py

A5rocks · 2024-09-06T17:10:54Z

@njsmith cause this is interface design, do you think there's a better way to express all of this?

A5rocks · 2024-09-07T08:24:00Z

src/trio/_core/_parking_lot.py

+        contains a reference to the task sent as a parameter."""
+        self.broken_by = task
+
+        # TODO: is there any reason to use self._pop_several?


Probably some thin veneer of thread safety? Or guaranteeing that we'll remove every task we raise an error in, even if somehow _core.reschedule raises an error (... well, we'll remove the task where this happens too. Oh well, still better behavior).

Neither is really something we care about but probably worth using cause it's a simple drop in replacement anyways.

unpark() above would have the same theoretical issue, if reschedule() failed it could drop tasks too. Probably not something to worry about, if that fails we've got bigger problems.

src/trio/_core/_parking_lot.py

jakkdl · 2024-09-10T11:35:50Z

@njsmith cause this is interface design, do you think there's a better way to express all of this?

as we don't actually have anybody requesting the functionality on ParkingLot or add_parking_lot_breaker/remove_parking_lot_breaker, we could opt not to make these public for now and document them as an experimental API. Or I suppose make them public but with a warning text in the docstring that they're experimental.

…as it in acquire, update docstrings. docs are failing to build locally but idk wth is wrong

newsfragments/3081.feature.rst

Zac-HD

This looks good to me! I'd appreciate a review by another experienced maintainer but I'd be inclined to merge once minor comments are addressed 😁

src/trio/_sync.py

src/trio/_core/_parking_lot.py

newsfragments/3081.feature.rst

src/trio/_core/_tests/test_parking_lot.py

src/trio/_core/_parking_lot.py

Zac-HD

Two minor things below, and then unless @njsmith or @oremanj raise any concerns let's merge after the weekend!

src/trio/_core/_run.py

src/trio/_sync.py

A5rocks

Mostly just nitpicks

A5rocks · 2024-09-19T22:38:07Z

src/trio/_core/_parking_lot.py

+    """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.
+    """


These should have a summary.

I think it might be better to link to the break_lot to explain the effects of a task exiting without removing themselves.

A5rocks · 2024-09-19T22:39:38Z

src/trio/_core/_parking_lot.py

+        """Break this lot, causing 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.
+        """


Summary line here too. Also "The error raised contains a reference to the task sent as a parameter." should probably just mention that broken_by is available on parking lots, because I personally missed that previously.

A5rocks · 2024-09-19T22:41:43Z

src/trio/_core/_run.py

@@ -1820,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)


Maybe add a line comment here like # break parking lots associated with a task that exited because otherwise the warning (from multiple tasks breaking a lot) might point to here in the stack trace.

A5rocks · 2024-09-19T22:44:59Z

src/trio/_core/_tests/test_parking_lot.py

+        await lot.park()
+
+    lot = ParkingLot()
+    with RaisesGroup(Matcher(_core.BrokenResourceError, match="Parking lot broken by")):


Suggested change

with RaisesGroup(Matcher(_core.BrokenResourceError, match="Parking lot broken by")):

with RaisesGroup(Matcher(_core.BrokenResourceError, match="^Parking lot broken by")):

for consistency

A5rocks · 2024-09-19T22:46:05Z

src/trio/_sync.py

 from ._core import Abort, ParkingLot, RaiseCancelT, enable_ki_protection
+from ._core._parking_lot import add_parking_lot_breaker, remove_parking_lot_breaker


We could import the functions from ._core for a nicer import

A5rocks · 2024-09-19T22:46:26Z

src/trio/_sync.py

+                await self._lot.park()
+            except trio.BrokenResourceError:
+                raise trio.BrokenResourceError(
+                    "Owner of this lock exited without releasing: {self._owner}",


Suggested change

"Owner of this lock exited without releasing: {self._owner}",

f"Owner of this lock exited without releasing: {self._owner}",

A5rocks · 2024-09-19T22:48:00Z

src/trio/_tests/test_sync.py

+
+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


How about testing the full fix because this PR should be the full fix.

initial implementation of lot breaking

3bd67a5

jakkdl mentioned this pull request Sep 6, 2024

Problems with trio.Lock() across multiple tasks #3035

Open

fix import cycle

4dfa1ad

jakkdl added 2 commits September 6, 2024 12:47

fix re-export for verifytypes visibility

543a087

update docstrings

c36cdad

TeamSpen210 reviewed Sep 6, 2024

View reviewed changes

src/trio/_core/_parking_lot.py Outdated Show resolved Hide resolved

TeamSpen210 reviewed Sep 6, 2024

View reviewed changes

src/trio/_core/_parking_lot.py Outdated Show resolved Hide resolved

jakkdl added 5 commits September 6, 2024 13:15

fixes after review by TeamSpen210

127c5fc

add tests

1f75d44

add lock handover test

6835e87

clean up breaker dict

3b86e80

clean up GLOBAL_PARKING_LOT_BREAKER when task releases or exits

94ff9a2

A5rocks reviewed Sep 6, 2024

View reviewed changes

src/trio/_core/_parking_lot.py Outdated Show resolved Hide resolved

src/trio/_core/_parking_lot.py Outdated Show resolved Hide resolved

src/trio/_core/_parking_lot.py Outdated Show resolved Hide resolved

A5rocks reviewed Sep 7, 2024

View reviewed changes

jakkdl commented Sep 10, 2024

View reviewed changes

src/trio/_core/_parking_lot.py Outdated Show resolved Hide resolved

jakkdl added 2 commits September 10, 2024 15:10

add newsfragments, add StalledLockError, reraise BrokenResourceError …

eb7a451

…as it in acquire, update docstrings. docs are failing to build locally but idk wth is wrong

add test for default argument of break_lot

e7d7205

jakkdl marked this pull request as ready for review September 10, 2024 13:14

Merge branch 'main' into break_the_lot

c89fb2a

jakkdl commented Sep 10, 2024

View reviewed changes

newsfragments/3081.feature.rst Outdated Show resolved Hide resolved

jakkdl requested review from njsmith and Zac-HD September 17, 2024 09:35

jakkdl mentioned this pull request Sep 17, 2024

raise BrokenResourceError if lock owner has exited. #3080

Open

Zac-HD reviewed Sep 17, 2024

View reviewed changes

A5rocks reviewed Sep 18, 2024

View reviewed changes

src/trio/_core/_parking_lot.py Show resolved Hide resolved

jakkdl mentioned this pull request Sep 18, 2024

During docs builds, leave newsfragments unchanged #3086

Merged

various fixes after review

277c7da

Merge remote-tracking branch 'origin/main' into break_the_lot

21cf0d6

jakkdl requested review from Zac-HD and A5rocks September 18, 2024 10:36

Zac-HD approved these changes Sep 19, 2024

View reviewed changes

src/trio/_core/_run.py Outdated Show resolved Hide resolved

src/trio/_sync.py Outdated Show resolved Hide resolved

break lots before other checks, minor phrasing improvement in docstring

45f78f4

A5rocks reviewed Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to break parking lots, stop locks from stalling #3081

Add ability to break parking lots, stop locks from stalling #3081

jakkdl commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading

A5rocks commented Sep 6, 2024

A5rocks Sep 7, 2024 •

edited

Loading

TeamSpen210 Sep 8, 2024

jakkdl commented Sep 10, 2024

Zac-HD left a comment

Zac-HD left a comment

A5rocks left a comment

A5rocks Sep 19, 2024

A5rocks Sep 19, 2024

A5rocks Sep 19, 2024

A5rocks Sep 19, 2024

A5rocks Sep 19, 2024

A5rocks Sep 19, 2024

A5rocks Sep 19, 2024

	with RaisesGroup(Matcher(_core.BrokenResourceError, match="Parking lot broken by")):
	with RaisesGroup(Matcher(_core.BrokenResourceError, match="^Parking lot broken by")):

		from ._core import Abort, ParkingLot, RaiseCancelT, enable_ki_protection
		from ._core._parking_lot import add_parking_lot_breaker, remove_parking_lot_breaker

	"Owner of this lock exited without releasing: {self._owner}",
	f"Owner of this lock exited without releasing: {self._owner}",

Add ability to break parking lots, stop locks from stalling #3081

Are you sure you want to change the base?

Add ability to break parking lots, stop locks from stalling #3081

Conversation

jakkdl commented Sep 6, 2024 • edited Loading

codecov bot commented Sep 6, 2024 • edited Loading

Codecov Report

A5rocks commented Sep 6, 2024

A5rocks Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Sep 10, 2024

Zac-HD left a comment

Choose a reason for hiding this comment

Zac-HD left a comment

Choose a reason for hiding this comment

A5rocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Sep 6, 2024 •

edited

Loading

codecov bot commented Sep 6, 2024 •

edited

Loading

A5rocks Sep 7, 2024 •

edited

Loading