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

Clean up / test / APIify the KeyboardInterrupt handling #16

Closed
njsmith opened this issue Jan 22, 2017 · 2 comments
Closed

Clean up / test / APIify the KeyboardInterrupt handling #16

njsmith opened this issue Jan 22, 2017 · 2 comments

Comments

@njsmith
Copy link
Member

njsmith commented Jan 22, 2017

This needs work in a few ways:

I'm not a big fan of how we currently respond to KeyboardInterrupt. Better idea: when the SIGINT handler fires, deliver a KeyboardInterruptCancelled exception (inherits from Cancelled, KeyboardInterrupt) to the current task immediately + all other tasks at their next cancellation point + set the special thing that makes new call_soon spawns get cancelled with this if they show up. This captures the idea that a KeyboardInterrupt is targeted at the whole process, not just one task! And then at the end the final exception should be a KeyboardInterrupt (or subclass) in most cases, rather than UnhandledExceptionError. (Maybe a strong supervision tree would help here, by routing all exceptions up through main rather than having them go straight to the crash machinery? Of course right now we literally send KeyboardInterrupt straight to the crash machinery!)

Also, there are no tests for the current KeyboardInterrupt handling.

Also, we need to provide some sort of reasonable public (hazmat) API for this stuff.

And finally we need to audit the codebase to make sure we're using this API to mark the correct things as protected. E.g. I'm pretty sure ParkingLot needs some annotations! And the run_in_trio_thread stuff is just wrong right now. (call_soon(..., spawn=True) needs to not enable interrupts, but currently it does; and then {run,await}_in_trio_thread need to enable them at the right point.)

@njsmith
Copy link
Member Author

njsmith commented Jan 23, 2017

This is now very much improved:

  • we now have some pretty good tests

  • there's a hazmat API

  • the things that should be protected are, I think (though this could probably use some more audits)

and, it's now switched to the model where the current task may-or-may-not receive an immediate KeyboardInterruptCancelled, and everyone receives one sooner or later. And we immediately raise a KeyboardInterrupt as a final "unhandled exception".

Remaining problems:

  • if a new task is spawned from call_soon after a KeyboardInterrupt, then it gets a regular cancellation, not a KeyboardInterruptCancellation. Maybe this is fine?

  • as part of the rework of how task crashes are handled, we probably want to make it so when these cancelled exceptions (like KeyboardInterruptCancelled, for example) propagate out the top of the task, they get suppressed, similar to have move_on_at and friends do things. Currently they get wrapped in UnhandledExceptionError and this overwrites the initial unhandled KeyboardInterrupt, so pytest still doesn't respond nicely to control-C during the test suite (because it ends up getting an UnhandledExceptionError). The details here are very dependent on Design: task supervision and results #7, though.

  • the immediate KeyboardInterrupt that we inject into the current task is not, right now, annotated with the special _stack_entry attribute that move_on_after uses to recognize a valid cancellation exception, and that whatever we come up with in the previous step will probably want to use as well. So that needs a solution too -- it should probably be treated the same as the KeyboardInterrupts that are delivered through the cancellation machinery.

@njsmith
Copy link
Member Author

njsmith commented Feb 12, 2017

This is pretty solid now.

@njsmith njsmith closed this as completed Feb 12, 2017
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

1 participant