Cancel scopes with disjoint extents #886
Comments
|
Hmm, here's a terrible idea: we could make the ...Yeah, that's a terrible idea, let's not do that. Way too magic. |
An idea I had post #835 was to have cancel scopes calculate a "merged deadline" = min(my deadline, my linked parent's merged deadline), and use that for effective deadline calculations. Then the linked parent doesn't have to be in the loop for cancellations that occur on a deadline, and we don't have to make an exception to the "cancel scopes in Runner.deadlines always have some tasks in them" rule.
I did this because at the time I was doing a lot of experiments with cancellation that involved toggling the shield status of multiple scopes at once, and linked children made for a convenient way to manage that. I don't think Trio itself needs to support that; it's easy to bolt on in external code. One other thought: an especial complexity of the linked children implementation in #835 was its need to carefully batch together all tasks that would be notified on any top-level operation, so that if two nested scopes were both cancelled, the outer one would win. I might take a stab at factoring this out into a
If we make
I actually think that would be a good solution, just because I can't imagine a situation where someone would want to introspect the |
This generalizes the "cancel multiple scopes without waking up the tasks, then wake up all the tasks at once" pattern that was previously a special case for handling cancellations on a deadline. It will simplify the implementation of python-trio#886, and exposing it as part of hazmat lets users write their own cancellation abstractions that work the way the native ones do.
This generalizes the "cancel multiple scopes without waking up the tasks, then wake up all the tasks at once" pattern that was previously a special case for handling cancellations on a deadline. It will simplify the implementation of python-trio#886, and exposing it as part of hazmat lets users write their own cancellation abstractions that work the way the native ones do.
This generalizes the "cancel multiple scopes without waking up the tasks, then wake up all the tasks at once" pattern that was previously a special case for handling cancellations on a deadline. It will simplify the implementation of python-trio#886, and exposing it as part of hazmat lets users write their own cancellation abstractions that work the way the native ones do.
Relevant to python-trio#886, python-trio#606, python-trio#285, python-trio#147, python-trio#70, python-trio#58, maybe others. I was continuing my effort to shoehorn linked cancel scopes and graceful cancellation into `CancelScope` earlier today and it was feeling too much of a mess, so I decided to explore other options. This PR is the result. It makes major changes to Trio's cancellation internals, but barely any to Trio's cancellation semantics -- all tests pass except for one that is especially persnickety about `cancel_called`. No new tests or docs yet as I wanted to get feedback on the approach before polishing. An overview: * New class `CancelBinding` manages a single lexical context (a `with` block or a task) that might get a different cancellation treatment than its surroundings. "All plumbing, no policy." * Each cancel binding has an effective deadline, a _single_ task, and links to parent and child bindings. Each parent lexically encloses its children. The only cancel bindings with multiple children are the ones immediately surrounding nurseries, and they have one child binding per nursery child task plus maybe one in the nested child. * Each cancel binding calculates its effective deadline based on its parent's effective deadline and some additional data. The actual calculation is performed by an associated `CancelLogic` instance (a small ABC). * `CancelScope` now implements `CancelLogic`, providing the deadline/shield semantics we know and love. It manages potentially-multiple `CancelBinding`s. * Cancel stacks are gone. Instead, each task has an "active" (innermost) cancel binding, which changes as the task moves in and out of cancellation regions. The active cancel binding's effective deadline directly determines whether and when `Cancelled` is raised in the task. * `Runner.deadlines` stores tasks instead of cancel scopes. There is no longer a meaningful state of "deadline is in the past but scope isn't cancelled yet" (this is what the sole failing test doesn't like). If the effective deadline of a task's active cancel binding is non-infinite and in the future, it goes in Runner.deadlines. If it's in the past, the task has a pending cancellation by definition. Potential advantages: * Cancellation becomes extensible without changes to _core, via users writing their own CancelLogic and wrapping a core CancelBinding(s) around it. We could even move CancelScope out of _core if we want to make a point. * Nursery.start() is much simpler. * Splitting shielding into a separate object from cancellation becomes trivial (they'd be two kinds of CancelLogic). * Most operations that are performed frequently take constant time: checking whether you're cancelled, checking what your deadline is, entering and leaving a cancel binding. I haven't benchmarked, so it's possible we're losing on constant factors or something, but in theory this should be faster than the old approach. * Since tasks now have well-defined root cancel bindings, I think python-trio#606 becomes straightforward via providing a way to spawn a system task whose cancel binding is a child of something other than the system nursery's cancel binding. Caveats: * We call `current_time()` a lot. Not sure if this is worth worrying about, and could probably be cached if so. * There are probably bugs, because aren't there always? Current cancel logic: ``` def compute_effective_deadline( self, parent_effective_deadline, parent_extra_info, task ): incoming_deadline = inf if self._shield else parent_effective_deadline my_deadline = -inf if self._cancel_called else self._deadline return min(incoming_deadline, my_deadline), parent_extra_info ``` Want to support a grace period? I'm pretty sure it would work with something like ``` def compute_effective_deadline( self, parent_effective_deadline, parent_extra_info, task ): parent_cleanup_deadline = parent_extra_info.get("effective_cleanup_deadline", parent_effective_deadline) if self._shield: parent_effective_deadline = parent_cleanup_deadline = inf my_cleanup_start = min(self._deadline, self._cancel_called_at) merged_cleanup_deadline = min(parent_cleanup_deadline, my_cleanup_start + self._grace_period) my_extra_info = parent_extra_info.set("effective_cleanup_deadline", merged_cleanup_deadline) if self._shield_during_cleanup: effective_deadline = merged_cleanup_deadline else: effective_deadline = min(parent_effective_deadline, my_cleanup_start) return effective_deadline, my_extra_info ``` Maybe that's not quite _simple_ but it is miles better than what I was looking at before. :-)
Well.... ok, you got me thinking about it more :-). But this made me realize another possible problem: I can't think how to make this work in a way that doesn't potentially leak memory. Maybe endowing each
Well, with 3.8 we can do scope = CancelScope(...)
with scope as was_cancelled:
...That's annoying if you really do write it that way, but it seems unusual that we'd want to create a scope → immediately enter it → then mutate it from inside the block. I can think of cases where you want to create a scope in one place and then enter it somewhere else, but that wouldn't be a problem here. And I can think of cases where you want to set some kind of deadline, so you do Oh, but that is a problem: where do nurseries keep the Stepping back: this is pretty complicated. Probably we shouldn't commit to a solution for disjoint extents right this moment. Maybe we'll even decide we don't need this feature to be built-in at all. The urgent question is: are we OK with releasing the current unbound We've always had cancel scope objects with If we do keep returning Option 1: maybe we'll never add built-in support for "linked" cancel scopes. You can get a similar effect pretty easily in Trio right now: just make a class that hands out Option 2: if we do commit to keeping However! This did make me realize: one of the cute ideas for But with the So... I'm thinking for right now (i.e., before we make the next release!), I'd like to switch to saying that a |
To keep our options open for adding a smarter `was_cancelled` property, per discussion in python-trio#886.
So! Linked cancel scopes! This is an idea that was first raised in #607 (comment), and also discussed in #835. The latter also has some notes on implementation subtleties that I won't copy here, but are worth reviewing if/when this turns into a PR again.
Here's the problem: If you want to have a single
CancelScopethat applies to multiple disjoint tasks, then how do you do that? Even with unbound cancel scopes (#835), you can't, because aCancelScopetracks information about both what's been requested, and what actually happened to a specific block of code (.cancelled_caught, or maybe.cancel_succeeded, see #885). If the same scope object could be used for multiple pieces of code simultaneously, then this wouldn't work.One idea is to let a cancel scope fork off "linked children": new
CancelScopeobjects that are their own independent entity, but that whenever the parent is cancelled this automatically cancels the children as well.(Do we have any example use cases? I feel like we did, but I don't remember them off the top of my head.)
There's a lot about this that I like, but there are also a lot of potential complexities to think through:
Introspection: should there be a way to get the parent scope? The child scopes? The descendent scopes? The initial version of Add support for unbound cancel scopes #835 had a
.linked_childrenattribute that returned all descendents.current_effective_deadline: I guess this would need to walk up the parent chain for each cancel scope to collect up all their deadlines? (Which means we need to store parent links internally.)How do shield attributes and linked scopes interact? In the initial version of Add support for unbound cancel scopes #835, changes to
parent.shieldpropagated tochild.shield, but this felt very weird to me. Conceptually, the shield component of a cancel scope is pretty much disjoint from the cancellation component. Maybe this is more evidence for the idea raised in idea: unbound cancel scopes #607, that shielding should be split off into a separate object from cancel scopes?If we add a "soft" cancellation concept (More ergonomic support for graceful shutdown #147), then how will that interact with this?
How tightly coupled should the parent and the children be? The two possibilities that come to mind are: (a) the idea described above, where the children are totally independent, full-fledged
CancelScopeobjects, except thatparent.cancel()triggers a call tochild.cancel(). (b) making them "linked clones", where the parent's deadline is the child's deadline, mutating one deadline mutates them all, cancelling any of them cancels them all, etc. The only thing that would be independent is.cancelled_caught, and maybe in the future some kind of data about what exceptions were caught.I guess it won't make a big difference if the way you actually use them in practice is always one of these two stereotyped patterns:
.....ugh now that I write that I'm wondering if we should revisit the discussion in #607 about whether
cancelled_caughtshould really be part of the cancel scope itself, versus something separate that you get from__enter__. The main argument against splitting them is that it would make a mess of helpers likemove_on_afterandfail_after, where if you wanted to expose both the scope object (to allow e.g. adjusting deadlines) and thecancelled_caughtobject (very important formove_on_after), then having them on two separate objects is annoying, and also compatibility breaking unless we introduce yet a third object, and then Occam starts jumping up and down and waving his arms.BUT. Here's something we should at least consider:
CancelScopeis now becoming a full-fledged class that regular users will interact with. And we could trivially give its constructor atimeout=argument. Supposedlymove_on_atandmove_on_afterare convenience shorthands, but at that point they're not adding a lot of convenience. Andfail_at/fail_afterare of dubious value to start with, + if you're using them you certainly don't care about the.cancelled_caughtattribute.So here is a possibility we should at least consider:
But the key idea here is that the
CancelScopeobject doesn't expose state related to this particularwithblock, so you could also writecancel_scope = CancelScope(...)and then enter it as many times as you wanted, where-ever you wanted.fail_after/fail_atcould stay like they are now, or maybe become.enter(raise_on_cancel=True)?I guess
was_cancelled.__bool__would raise an exception if you called it from inside the associatedwithblock?(While we're at it: consider globally renaming
deadline→at,timeout→aftereverywhere.)And hmm, I was thinking that we needed the
.enter()there so that we could match up the calls to__enter__and__exit__, so we knew whichwas_cancelledobject to update. But actually it would be trivial to stash this information on the task, e.g. in the cancel stack. So we could drop the.enter().If we do decide to provide an option to capture the
Cancelledexceptions for later introspection, then that would need something likewith cancel_scope.saving_exceptions() as was_cancelled: ..., orwith cancel_scope.using(save_exceptions=True) as was_cancelled: ..., but that seems fine.I don't know if this is a good idea. It's certainly a bigger change than I'd like to be making at this point, now that people are starting to build real projects on top of Trio. But... well, I thought it, so I'm writing it down, and we'll see what you all think (where "you" includes "me, but tomorrow").
The text was updated successfully, but these errors were encountered: