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

Raise deferred KeyboardInterrupt out of run() after cancelling all tasks #1537

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

oremanj
Copy link
Member

@oremanj oremanj commented May 19, 2020

Fixes #151, first step of #733.

@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #1537 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1537      +/-   ##
==========================================
- Coverage   99.67%   99.67%   -0.01%     
==========================================
  Files         108      108              
  Lines       13358    13345      -13     
  Branches     1012     1008       -4     
==========================================
- Hits        13315    13302      -13     
  Misses         28       28              
  Partials       15       15              
Impacted Files Coverage Δ
trio/_core/_traps.py 100.00% <ø> (ø)
trio/_core/_exceptions.py 100.00% <100.00%> (ø)
trio/_core/_io_kqueue.py 83.01% <100.00%> (ø)
trio/_core/_io_windows.py 98.54% <100.00%> (ø)
trio/_core/_run.py 99.72% <100.00%> (-0.01%) ⬇️
trio/_core/tests/test_ki.py 100.00% <100.00%> (ø)
trio/_core/tests/test_run.py 100.00% <100.00%> (ø)

@oremanj oremanj changed the title [WIP] Inject deferred KeyboardInterrupt in a new task, instead of waking up the main task to deliver it Raise deferred KeyboardInterrupt out of run() after cancelling all tasks May 22, 2020
@oremanj oremanj requested a review from njsmith May 22, 2020 10:00
@oremanj
Copy link
Member Author

oremanj commented May 22, 2020

The coverage "failure" is because trio._core._run now contains fewer statements, so the same set of uncovered statements results in a lower coverage percentage.

@pquentin
Copy link
Member

Sorry for the merge conflict, I just merged the Black pull request

@njsmith
Copy link
Member

njsmith commented May 23, 2020

I'm not sure cancelling the system nursery is a great idea... until now, we had an invariant that system tasks would only be cancelled (a) after the main task exited, or (b) after a system task crashed. So they basically had the "service task" semantics discussed in #1521.

I feel like KeyboardInterrupt should immediately cancel the main task, but not immediately cancel system tasks.

@oremanj
Copy link
Member Author

oremanj commented May 23, 2020

That makes sense to me, but I think it will be easiest to implement by just doing #1521, because I don't see any way to wrap the main task in a dedicated cancel scope without adding another frame to every Trio traceback.

@njsmith
Copy link
Member

njsmith commented May 23, 2020

Maybe possibly we could do something like this?

    async def init(self, async_fn, args):
        async with open_nursery() as system_nursery:
            self.system_nursery = system_nursery
            async with open_nursery() as main_task_nursery:   ##### <-----
                try:
                    self.main_task = self.spawn_impl(async_fn, args, main_task_nursery, None)
                except BaseException as exc:
                    self.main_task_outcome = Error(exc)
                    system_nursery.cancel_scope.cancel()
                self.entry_queue.spawn()

but yeah, I see your point

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

Successfully merging this pull request may close these issues.

How should shielding interact with restrict_keyboard_interrupt_to_checkpoints?
3 participants