Skip to content

Defer KI if trio is doing IO #3233

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

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

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Mar 28, 2025

Supersedes #1537.

As with that, fixes #151, first step of #733.

Additionally, fixes part of #3007 (not intentionally, just because it has to be done in order to still raise a KI. I should add a test.).

@A5rocks
Copy link
Contributor Author

A5rocks commented Mar 28, 2025

codecov failure doesn't make any sense and seems to be a recent thing. I'm inclined to believe it's some change in how they normalize file names but I don't think it's worth chasing down for now.

Copy link

codecov bot commented Apr 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.75077%. Comparing base (ea03051) to head (f83ebbb).

Additional details and impacted files
@@                 Coverage Diff                  @@
##                 main       #3233         +/-   ##
====================================================
- Coverage   100.00000%   99.75077%   -0.24923%     
====================================================
  Files             127         127                 
  Lines           19266       19259          -7     
  Branches         1301        1300          -1     
====================================================
- Hits            19266       19211         -55     
- Misses              0          45         +45     
- Partials            0           3          +3     
Files with missing lines Coverage Δ
src/trio/_core/_run.py 99.61353% <100.00000%> (-0.38648%) ⬇️
src/trio/_core/_tests/test_cancelled.py 100.00000% <100.00000%> (ø)
src/trio/_core/_tests/test_guest_mode.py 100.00000% <100.00000%> (ø)
src/trio/_core/_tests/test_ki.py 100.00000% <100.00000%> (ø)
src/trio/_repl.py 100.00000% <100.00000%> (ø)

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the change to the abort parameter tightly bound to the other changes in this PR? Can it be pulled out into a separate PR?

This is a daunting PR to review, with one former PR and 4 related issues, and changes in 23 files, and it might be more manageable if it got split up.

It'd also be nice if the PR summarized the state of discussion, I don't know if KI-deferral has been unanimously agreed to be the better way to do it, or if there's up- & downsides.

Sorry for the non-review review <3

@A5rocks
Copy link
Contributor Author

A5rocks commented May 5, 2025

I can do the change to abort fn as a follow up -- it needs this change because the reason it's a function is because KI can be raised (which this change stops), but this change doesn't depend on it.

This isn't so much a summary but see #733 (comment) (this is ~just about that issue, other issues being closed is just because this is a Good Idea and unlocks easy fixes).

Copy link
Contributor Author

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I removed the abort function signature change.

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems fine as far as I can tell~

_core.run(check_protected_kill)

# TODO: be consistent about providing Cancelled tree as __context__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's required for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the issue here is that in the above test, no __context__ tree is kept which is wrong. Maybe.

But now that I think about it more, I think this might be fine? If a task is busy-looping it'll raise a KeyboardInterrupt too. I don't really like the Cancelled tree so maybe we should do the opposite of this and try to eliminate it! I'll update this to have this written down. (I'd rather know whether this tree of cancellation is actually a burden but I suspect it'll add many lines to tracebacks in event of ctrl+C for not much use)

CoolCat467 and others added 2 commits June 10, 2025 20:24
`pytest-trio` should error if `ki_pending` is `True` at the end of
execution...
@A5rocks
Copy link
Contributor Author

A5rocks commented Jun 29, 2025

Looking at #3030 makes me unsure whether this will work with that (specifically pressing ctrl+c when some async code is already running seems like it would break with this). I'd rather have that first, so I'll mark this as a draft.

I guess that PR doesn't actually touch the code I'm worried about, but I want to copy over whatever style of test case we figure out anyways.

@A5rocks A5rocks marked this pull request as draft June 29, 2025 04:55
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?
5 participants