How should shielding interact with restrict_keyboard_interrupt_to_checkpoints?

[njsmith]

Right now KeyboardInterrupt can (if using the default handler) arrive anywhere; restrict_keyboard_interrupt_to_checkpoints=True makes it so it can only arrive at checkpoints. But in principle it can arrive at any checkpoint. In practice it currently only arrives at checkpoints inside the main task, which usually is going to be sitting in supervisor code. And currently it ignores shield=True scopes – you can get KeyboardInterrupted even if you're shielded against Cancelled.

The nursery clean-up code is prepared to handle this correctly, and that's what the main task will be doing in most cases, so this isn't super urgent. But it is surprising, maybe? Is it correct?

[oremanj]

Closing this in favor of the discussion in #733. It sounds like the plan is to switch to always raising KeyboardInterrupt-at-checkpoint in a new task, so that all existing tasks experience it as a cancellation, which can be shielded against normally.

[oremanj]

We might be injecting it in a new task, but if we put the new task in a nursery that's surrounded by a shielded cancel scope, it will still behave idiosyncratically: the other tasks in that nursery will be cancelled due to what is effectively a top-level cancellation, even though there's a shield between them and the top level.

Reopening this issue to track that. Probably the solution is to not inject the KI task in any nursery that would not experience a top-level cancellation.

[njsmith]

I'm thinking the simplest would be to always inject the KI task into the top-level system nursery, or thereabouts... there's some awkward code currently to handle the main task and its result + TrioInternalErrors, so it might take some thinking to figure out how to refactor that, but in the end I think it'll be simpler than trying to pick random victim nurseries, take shielding into account, etc.

…

On Wed, May 20, 2020 at 2:02 AM Joshua Oreman ***@***.***> wrote: We might be injecting it in a new task, but if we put the new task in a nursery that's surrounded by a shielded cancel scope, it will still behave idiosyncratically: the other tasks in that nursery will be cancelled due to what is effectively a top-level cancellation, even though there's a shield between them and the top level. Reopening this issue to track that. Probably the solution is to not inject the KI task in any nursery that would not experience a top-level cancellation. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#151 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEU42EZXJZZ6SLTIMGJTODRSOMCLANCNFSM4DKODZSQ> .

-- Nathaniel J. Smith -- https://vorpus.org <http://vorpus.org>

[oremanj]

If you're good with that, then I agree it's much easier! My rationale for picking the innermost main task nursery in #1537 was that it's the most likely to match the point where current Trio raises KI, so most likely to be backwards-compatible with people trying to catch KI within Trio. If we think that doesn't matter (beyond notifying #1 and mentioning in the release notes), then let's take the simpler approach.

[njsmith]

Eh, I wouldn't say it "doesn't matter" exactly – it would be nice if we could inject the KeyboardInterrupt inside the main task. But it introduces a ton of complicated edge cases: what if there's no main-task nursery? what if the main task has keyboard interrupt protection enabled? What if the main task is shielded? what if the user did trio.run(..., keyboard_interrupt_is_always_cancel=True)? and so on and so on...

And it's easy to justify in docs: say a KI arrives while a system task is running; why should that get diverted "sideways" into a sibling task that wasn't running? Having it come out of trio.run instead is pretty logical.

And I think the required changes to user code should be fairly minor?

So on net I think the disruption is justified. Though we'll see what users think :-)

[smurfix]

FWIW, I tend to agree. Most likely, the interrupt occurs while Trio is waiting on epoll/select/io_uring/whatever, and if it doesn't we can just assume that it did because it's all async anyway. ;-)

If we really want to divert a KI to some task, the "main" task is the last task I'd expect the interrupt to be diverted to; a much more helpful candidate would be whichever task ran last (and didn't terminate of course), on the grounds that this is the one that's most likely to be the task of interest, from the PoV of somebody scratching their head when they squint at the traceback.

However, that (as @njsmith mentioned) has a ton of edge cases which we could of course somehow handle, but why bother? The effort would be much better spent by implementing some task monitor-ish code which, when turned on, would print all running tasks and their tracebacks. That kind of introspectability would be helpful in a lot of other cases …

I agree that there should be few or no changes to user code. You shouldn't explicitly handle KeyboardInterrupt within async code anyway, that's what BaseException is for.

[oremanj]

The logic would be: Iterate the main task's nurseries from innermost outward. If you find a nursery in which it would be reasonable to raise KI (open, unshielded, not KI-protected), then inject the KI there. Otherwise, inject it at system task level.

It's not too complex to implement, but it definitely does create a number of additional cases to test, including the rather thorny "KI arrives during the one-tick interval between a nursery being closed and that nursery being removed from parent_task.child_nurseries". I'd just as soon go with the simpler approach if everyone is happy with the backcompat tradeoffs. But the more complex approach is feasible too (and mostly implemented already) if the complexity is thought to be worth it. :-)

[njsmith]

Otherwise, inject it at system task level.

The thing is... The one downside of always injecting it at the system task level is that users need to move their except KeyboardInterrupt blocks out around trio.run. But... If we can sometimes inject it there, then they have to do the same thing. So if we're going to do it at all, then we might as well do it all the time.

[oremanj]

So if we're going to do it at all, then we might as well do it all the time.

Makes sense! I've updated #1537 to take this approach.