-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Discussion: Cancel-myself-now operation? cancel-and-wait-for-it-to-finish operation? #658
Comments
this documentation adds to the confusion:
which suggests that |
is there a way to make |
We should definitely be clearer about If we do add a method that does cancel+checkpoint, then the cancel scope would be a more obvious place for it than the nursery, because it's generic cancellation-related functionality.
I'm having some trouble visualizing this... It's one of those times where the use case is super clear in your head because you're looking at it, but the rest of us have a bit more trouble :-). I guess you're... starting a task, and also passing in the nursery where |
Nurseries can be nested, so how would trio know which nursery you were throwing the |
Assuming we added another cancel-like exception for this case, it would apply to the first nursery catching it. That manager would convert it to a cancel scope + checkpoint.
The specific use case was was trying to upgrade this task in trio-websocket to used the start() protocol, and have it call started() after the HTTP handshake is done. Instead of the |
Agreed that it's probably better to do the handshake before starting that task. But also: I don't see why you can't just call |
We really try to avoid constructs like this, where the result depends on some subtle aspect of the context where the code is called. It creates non-obvious coupling and breaks compositionally/separation of concerns. And this case is actually a really great example of that :-). As it turns out, We can get away with this, because there's no way for user code to tell that there's a temporary nursery in there (modulo debugging APIs). With your |
Thank you for the explanation. I think the priorities are:
|
Well, we could have Probably not a good idea from the PoV of verifying code correctness, but … |
Confusing the matter further, the "cancel and immediately raise" operation only makes sense if it's called from within the cancel scope. In general, one task can cancel some work in another task just fine, but it wouldn't make sense to perform a naive implementation of this "cancel immediately" operation across tasks. If we go with In general trio tends to eschew The other possible API would be something that does I'd prefer not to reuse the term "abort" as that already has a meaning in trio (waking up a sleeping task to deliver a Cancelled or a KeyboardInterrupt exception). |
Having a separate async cancel method that does the right thing both inside and outside of the cancel scope sounds great. The naming will have to be a compromise, |
Interesting idea. This would interact awkwardly with #607, since an unbound cancel scope might never be entered, and thus never exited either. My intuition is that if we have to pick, then unbound cancel scopes are more useful than the ability to wait for a cancel scope to exit, mostly because I've noticed cases where I wanted unbound cancel scopes but not noticed cases where I wanted to wait for a cancellation. But maybe that's because I was looking :-). I still don't have any intuition for why |
Meh. If you're inside the scope to be cancelled, this doesn't wait – it raises I don't want a single method that affects control flow in two very different ways (and is named for only one of them); if we do that, we might as well have a If we don't want that (and I'm no longer sure whether keeping three names in mind is any worse than keeping three ways of using
Well, I'd wait only for the tasks to exit that are currently within the scope; if none are, return immediately. |
Just like anything else that blocks inside a cancelled cancel scope. :-) Though I understand the potential for confusion.
AIUI, unbound cancel scopes are intended to fix races where you might be cancelling before the scope gets entered, so having cancel_and_wait() return before the scope gets entered could be confusing. It seems hard to pick one behavior (wait for the scope to be exited VS return immediately if not entered yet) that would be intuitive in all cases.
I agree - I don't have specific use cases for "wait for cancel scope to exit" either, and it would entail some internal complexity / arguably some dependency inversion (cancel scopes currently don't have anything async going on). I'd support |
I think it's fair to revisit async options if some real use cases show up. +1 for "cancel and raise" method, though it really needs the RuntimeError safeguard mentioned by @smurfix for "I'm not within the scope being cancelled". |
Just had a case on my project where someone got burned thinking scope cancel is immediate. I'd like to confirm: any checkpoint will cause all pending cancel requests to be executed? |
Currently, yes, at every checkpoint, trio checks all the current cancel scopes, and if any of them are in the cancelled state then it raises Though now that you mention it... the theoretical guarantee should probably be a bit weaker than that. I suspect that we may eventually want to start skipping checkpoints sometimes as an efficiency measure. So the theoretical guarantee should be something like, "as long as your code executes checkpoints regularly, then cancellation will be delivered fairly promptly", but without guaranteeing that it will actually be delivered at the next checkpoint; maybe it will be after 2-3 checkpoints or something. I still don't have an intuition for why you want to be able to raise |
In this last case the code taking cancel action was actually outside of the cancel scope. Those weaker guarantees sound problematic. For example if something is managing tasks, and needs to cancel one and start another while ensuring that their lifetimes are mutually exclusive, it would be nice to just call cancel() and start_soon() and be done with it. That's the case now I believe since, on the next checkpoint, the scheduler will first processes cancels and then schedule new tasks. Or if that's too dependent on implementation, sure we can call sleep(0) after cancel to make sure the cancel is handled first. But if you're saying that may change to require multiple checkpoints to service outstanding cancels, we'd have to adopt more complex signaling for this case. |
It's true that right now the Trio scheduler advances by a series of "ticks", and in each tick it runs each runnable task forward by one checkpoint. But please don't rely on this as an inter-task synchronization mechanism. We definitely reserve the right to switch to more sophisticated scheduling algorithms in the future (see #32), and even if we don't, it's just incredibly fragile. Processing a cancellation can easily take multiple "ticks" for the exception to propagate (e.g. if you have any
You really don't want to have to do this kind of analysis to figure out if your code is correct. If the semantics you need are cancel-and-wait-for-cancelled-thing-to-exit, then you should implement something that has those semantics :-). Or like... if you need to make sure exactly one of these tasks is running at a time, can't you just have them grab a |
Thank you for shedding light on that fragility-- makes sense. In this particular case, use of locks in the task is not suitable. For the benefit of the task author's mental model, a task being run implies it has the green light to the resources it needs and won't block on things for indefinite time. This simplifies coding of this particular task type and reduces the necessary skill level of authors. Using locking is much different than general use of context managers to ensure things are restored to an expected state (which make take time) once the task ends or is cancelled. Given what you described, having an awaitable cancel sounds very useful. Please consider it. As it is I'll have to implement it myself by wrapping tasks and having a centralized object to wait on task completion. |
The challenge for awaitable cancel is what I said above... maybe it's confusing that you could have an awaitable cancel that ends up waiting forever because the cancel scope never gets entered? (Perhaps because the code that was going to enter it got cancelled by something else?) That doesn't mean I'm not considering it, I'm just sharing some of my considering :-). I guess another complexity is whether we want to allow cancel scopes to be re-entered. If we do, then the definition of "cancel scope has exited" becomes even more unclear. I guess we could avoid this by using the "linked" scope idea from the unbound cancel thread, but that idea makes waiting for a cancellation to finish even more poorly defined. As a general note, I appreciate that you've been trying to distill these cases down to their essence, and also that you might have reasons that you can't share too many details. But generally if you can, I'd actually prefer much more details rather than less... maybe this is just a personal thing, but I find it very difficult to weigh these kinds of design trade-offs without messy, detailed, concrete examples to think about. And right now I know of multiple concrete examples where unbound cancel scopes are useful (because I ran into them myself :-)), but don't have concrete examples of needing to wait for a cancellation to finish, so I find it hard to form an opinion either way... |
Hmm. "Cancellation finishes" == "no code is running within the cancel scope's context". Seems easy enough; trying to enter an already-cancelled scope would have to raise an |
In the current design, where all cancel scopes are associated with exactly one span of running code, then "has the code inside this scope finished?" is a perfectly well-defined question. AFAICT basically any way of relaxing that rule though leads to nasty snarls, that I suspect are unsolvable. Having an #607 also discusses other possible extensions of cancel scope lifetimes, like having a scope that you can exit and re-enter (imagine a loop where one part is subject to cancellation and another part is not), or entering the same scope multiple times simultaneously in different tasks. The specific proposal there is to continue to limit each This issues seem like they're pretty fundamental: if you give cancel scope users flexibility in defining the boundary of the abstract "operation" that you want to cancel, then it becomes hard to tell where that boundary is when you want to check whether the "operation" has finished. I guess we could turn it around, and say that "wait for this cancel scope to be exited" is only legal in the limited cases where it makes sense (e.g., the scope has to already be entered, can't have had any forks made, etc.). I'm not sure if that would be useful or not. |
I should say that in my recent case, the need to block on scope cancel() was thwarted. I reviewed the code and restructured it to avoid passing around nurseries or cancel scopes. (That's becoming my general principle with trio.) By making the program flow more "linear" I eliminated several cases where a cancel() call was needed. |
Right – the options seem to be
I'm leaning towards the second option. Creating an rwlock-like object that provides variations of "wait-until-no-more-tasks" is easy – and completely orthogonal to the concept of cancel scopes. So if you want such a thing, do it explicitly. Also, this way would allow for tasks that use the cancel scope but are ignored by the |
That sounds like a good outcome! There are times when passing around nurseries and cancel scopes is necessary, but it's always better to do the simpler thing when you can... |
I guess the cancel-and-wait-for-it-to-finish idea is dead now that unbound cancel scopes are merged (#835). And the cancel-myself-now operation doesn't sound like it's found any compelling use cases. (Also, why not just |
it isn't apparent from the docs that
cancel()
is non-raising, and that when you call it from within a nursery the program flow will continue until the next checkpointit seems like a common use case will be to abort from within a nursery scope using cancel() and sleep(0). Perhaps nursery should have a
cancel_now()
orabort()
methodmy use case was related to having a task meet the start() protocol invariant of either calling started or raising
The text was updated successfully, but these errors were encountered: