-
-
Notifications
You must be signed in to change notification settings - Fork 332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to break parking lots, stop locks from stalling #3081
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3081 +/- ##
=======================================
Coverage ? 99.60%
=======================================
Files ? 121
Lines ? 18004
Branches ? 3238
=======================================
Hits ? 17933
Misses ? 50
Partials ? 21
|
@njsmith cause this is interface design, do you think there's a better way to express all of this? |
src/trio/_core/_parking_lot.py
Outdated
contains a reference to the task sent as a parameter.""" | ||
self.broken_by = task | ||
|
||
# TODO: is there any reason to use self._pop_several? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
as we don't actually have anybody requesting the functionality on |
…as it in acquire, update docstrings. docs are failing to build locally but idk wth is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just nitpicks
"""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. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"""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. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
await lot.park() | ||
|
||
lot = ParkingLot() | ||
with RaisesGroup(Matcher(_core.BrokenResourceError, match="Parking lot broken by")): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with RaisesGroup(Matcher(_core.BrokenResourceError, match="Parking lot broken by")): | |
with RaisesGroup(Matcher(_core.BrokenResourceError, match="^Parking lot broken by")): |
for consistency
from ._core import Abort, ParkingLot, RaiseCancelT, enable_ki_protection | ||
from ._core._parking_lot import add_parking_lot_breaker, remove_parking_lot_breaker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could import the functions from ._core
for a nicer import
await self._lot.park() | ||
except trio.BrokenResourceError: | ||
raise trio.BrokenResourceError( | ||
"Owner of this lock exited without releasing: {self._owner}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Owner of this lock exited without releasing: {self._owner}", | |
f"Owner of this lock exited without releasing: {self._owner}", |
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about testing the full fix because this PR should be the full fix.
fixes #3035
TODO:
Lock
/_LockImpl
to get a better error message