Add ability to break parking lots, stop locks from stalling #3081
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3081 +/- ##
========================================
Coverage 99.58% 99.58%
========================================
Files 121 121
Lines 18002 18166 +164
Branches 3248 3275 +27
========================================
+ Hits 17927 18091 +164
Misses 52 52
Partials 23 23
|
|
@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.
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.
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
src/trio/_core/_parking_lot.py
Outdated
| """ | ||
| if task is None: | ||
| task = _core.current_task() | ||
| self.broken_by = task |
There was a problem hiding this comment.
I'm just now realizing that this relationship is a bit flawed. Lots can only be marked as broken by one task, but multiple tasks could be marked as a parking lot breaker and exit (or a task multiple times, but that's handled already). I'm not sure what's a good idea. We can't raise any error because there's no good place to raise errors, but I suppose we could warn? Or maybe broken_by should be a list.
There was a problem hiding this comment.
Having a list feels overkill, and only really being useful in the case of multiple independent coding errors all leading to them wanting to break a lot. It feels like what would more commonly happen is multiple instances of the same task breaking a lot for the same reason, in which case the list would just fill up with tons of duplicates. Or cases where once one task has broken a lot it causes multiple subsequent tasks to re-break it.
So I feel like the first breaker is ""special"" in most cases, and we'd rarely care about the others.
But raising a warning sounds good, maybe RuntimeWarning?
There was a problem hiding this comment.
should warning be raised if the same task breaks a lot multiple times?
There was a problem hiding this comment.
Yeah that sounds like a fine warning and a task breaking a lot multiple times is probably fine. After all we allow nesting add/remove_parking_lot_breaker
…docstring. fix runtimewarning transforming into triointernalerror. add a bunch of tests
| 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() |
There was a problem hiding this comment.
I think it's fine, the main issue I think is just warning in trio internals being misleading.
There was a problem hiding this comment.
I concur that this seems fine; we do want to give the warning but it's not otherwise a problem - in typical usage you'd get an error about the lot being broken when you go to park in it, and that will likely still happen here (if it hasn't already!).
src/trio/_core/_run.py
Outdated
| except RuntimeWarning: | ||
| raise | ||
| except TrioInternalError: | ||
| raise |
There was a problem hiding this comment.
This could be too broad (maybe warnings should cause internal errors and prompt people to open issues) or too narrow (if passing through runtimewarnings, why not all warnings?). Could make the RuntimeWarning a custom warning class instead
There was a problem hiding this comment.
I'd avoid special-casing this at all; if users set -Werror they're signing up to deal with this kind of thing or changing their code to avoid it.
There was a problem hiding this comment.
Hmm, I'm torn on this - in large part because TrioInternalError prompts users to open an issue to report it, and running pytest with -Werror isn't that obscure
There was a problem hiding this comment.
Oh, I see! Yeah, keeping it with a custom warning type seems good then.
There was a problem hiding this comment.
To be honest custom warning type is unnecessary if above lot.break_lot(task) we put a lot.broken_by check. I'm not sure that's indicative of quality of idea though.
There was a problem hiding this comment.
Requiring that check (and erroring on attempts to break an already-broken lot) seems pretty sensible to me, it's not like ParkingLot is an interface that gets called directly very often, and so I don't mind adding some ceremony if that helps catch tricky concurrency bugs.
There was a problem hiding this comment.
okay so with the check we no longer raise any warnings on task exit if multiple tasks try to break a task, which means we only ever give a warning if manually breaking a broken lot. That doesn't seem particularly logical to me, and we should probably do one of:
- make breaking a broken lot a no-op
- raise an error
- ...go with
broken_bybeing a list (or tuple).
The warning was added after this convo: https://github.com/python-trio/trio/pull/3081/files/7a1ce5b755b5775153a3525bea793ab7da05fb44#r1764544347 where I argued against the list, but if we no longer want to raise a warning from the internals then maybe the list is the way to go? (that also allows for duplicates in the list).
My gut feeling would be 3>1>2, but idk.
There was a problem hiding this comment.
I agree with that ordering (though I might slightly prefer 2 over 1? doesn't matter.)
There was a problem hiding this comment.
Only action items left AFAICT are to remove the except RuntimeWarning block, and optionally to clean up any comments that you want to adjust. Then let's merge 🚀
Also, thanks again @jakkdl - this has been a rather long project to fix a relatively small issue, but I think we've ended up with a really nice solution and really appreciate all your work on this 😍
src/trio/_core/_run.py
Outdated
| except RuntimeWarning: | ||
| raise | ||
| except TrioInternalError: | ||
| raise |
There was a problem hiding this comment.
I'd avoid special-casing this at all; if users set -Werror they're signing up to deal with this kind of thing or changing their code to avoid it.
| 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() |
There was a problem hiding this comment.
I concur that this seems fine; we do want to give the warning but it's not otherwise a problem - in typical usage you'd get an error about the lot being broken when you go to park in it, and that will likely still happen here (if it hasn't already!).
We can fix |
|
Made |
fixes #3035
TODO:
Lock/_LockImplto get a better error message