Skip to content
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

Some way to take a cancellation in one call to wait_task_rescheduled and forward it to another task #642

Open
njsmith opened this issue Sep 2, 2018 · 5 comments

Comments

@njsmith
Copy link
Member

njsmith commented Sep 2, 2018

In both #266 (shared tasks) and #606 (propagating cancellation across thread re-entry), we have a situation where we're pushing work from one place to another, so to Python (and the Trio core) it looks like two independent tasks, but logically it's one stack split over multiple tasks. Specifically:

  • One task schedules some work, and blocks in wait_task_rescheduled to wait for it to finish
  • Eventually, the work ends up being done in a system task, with exceptions etc. routing back to the original task
  • If the wait_task_rescheduled gets aborted, we want to propagate the cancellation to the system task, so that the cancellation actually works, and so that it gets raised at the appropriate place (the actual bottom of our stack), and ends up with a proper traceback

This is quite difficult currently.

What we want conceptually is like... to be able to open a cancel-scope-or-something-like-it in the system task, and then somehow "forward" cancellations from the original context to it. It's not immediately obvious to me how to do this with the current cancellation code – in particular there are subtleties like, if the parent context is cancelled normally, then we should put the child into a persistent cancelled state, and keep attempting to redeliver Cancelled with the parent's scope. If the parent has a KeyboardInterrupt delivered, we should try to forward on the raise_cancel logic to the child, so that if-and-only-if it accepts the exception, the pending_ki flag gets cleared.

@njsmith
Copy link
Member Author

njsmith commented Jun 13, 2019

See also: #961 (comment)

@oremanj
Copy link
Member

oremanj commented Jun 7, 2020

I started working on an implementation of this where each task has a "cancellation parent" which is a nursery or other task that determines where in the CancelStatus tree this task is attached. Normally the cancellation parent is the task's parent nursery, which gives the usual Trio semantics, but you can also set it to another task, and then you get the semantics discussed here. For example, the system task created by to_thread.run() would set its cancellation parent to the original task that had made the from_thread.run_sync() call to enter the thread. The expectation is that the task's outcome will be captured and reraised in its cancellation parent.

To support most of the interesting use cases, the cancellation parent has to be modifiable during the task's lifetime:

  • from_thread.run_sync(..., cancellable=True) has to be able to detach any child tasks when the parent abandons the thread.
  • Shared tasks should be attached nowhere as long as there are multiple requestors, but if there's one requestor then that should be the shared task's cancellation parent.
  • That also allows simplifying nursery.start() to use the cancellation parent mechanism + some logic in _child_exited() of the target nursery (to forward the task's outcome to the task blocked in start() if started() hasn't been called yet).

But having the cancellation parent be modifiable turns out to create a number of challenges:

  • It's difficult to fully ensure that a Cancelled exception isn't already propagating based on a cancellation in the old environment that doesn't exist in the new one. The nursery.start() logic of "don't move if the current context is cancelled" is both insufficient (there's an edge case where Cancelled is raised but then an outer scope becomes shielded making the current context effectively un-cancelled -- start() has this problem too) and overly strict (doesn't let you implement to_thread.run_sync(..., cancellable=True) -- by the time the outer task receives the cancellation, the inner system task has too, but the outer task wants to detach it and move on with its life, which is safe since the outcome of the inner system task is going to be thrown out anyway).
  • You have to worry about not creating cycles in the CancelStatus tree.
  • There's no node in the CancelStatus tree for "task", so if you have a chain of tasks where each is the cancellation parent of the next with no intervening cancel scopes (easy to obtain with a series of nursery.start() calls) then you need a lot of extra bookkeeping to figure out what you need to do to the CancelStatus tree when one of them changes its cancellation parent.

All of these are resolvable, but the solutions add complexity, so I figured it was worth checking back to see whether this is something we care about at all? #606 can be resolved by running reentrant child tasks in the original parent task rather than as system tasks, #266 can be done without core cooperation, and nursery.start() is fine as-is. If we think it's worth it, I can complete the cancellation_parent work and upload a PR; if not, probably best to just close this issue.

@oremanj
Copy link
Member

oremanj commented Jun 7, 2020

Typing that all out made me think of a better abstraction, though!

# Parent task:
with trio.lowlevel.open_cancel_portal() as portal:
    # Make portal object available to the virtual child task(s) somehow
    await trio.lowlevel.wait_task_rescheduled(...)
# Leaving the parent's context manager deactivates the portal in the children

# Child task (running as system task or something):
with portal.open_child():
    # Code in here can become cancelled based on the parent's state,
    # and the Cancelled exceptions will propagate out.
    # If the parent exits its open_portal() context, then the link is broken.
    # If a cancellation was already delivered, then the open_child() context will swallow it.

I think the main argument for this, rather than the alternative of manual propagation of cancellations, is that we can make it not break if/when we add more cancellation features (graceful, etc).

@njsmith
Copy link
Member Author

njsmith commented Jun 7, 2020

I've come around to thinking that maybe this is the wrong problem to try to solve, and we should handle from_thread by just... making the incoming task a child of the task that called to_thread. If we need this feature, what you wrote seems like a plausible API, but I'm not sure the feature is fully motivated yet.

(I know there's an issue somewhere that talks about making from_thread tasks children of to_thread, but I don't know where off the top of my head.)

@njsmith
Copy link
Member Author

njsmith commented Jun 10, 2020

Oh and for to_thread.run_sync(..., cancellable=True) (I'm assuming this is what you meant, not from_thread?), I think the answer is just that we shouldn't propagate cancellation or the trio token into those threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants