Track KeyboardInterrupt protection per code object

[oremanj]

Example implementation for the first idea from my comment on #733. Still somewhat unpolished (needs a newsfragment, doc updates, and additional tests) but I didn't want to put more work into it without feedback on the overall direction.

[codecov[bot]]

Codecov Report

Merging #1508 into master will decrease coverage by 0.08%. The diff coverage is 89.32%.

@@            Coverage Diff             @@
##           master    #1508      +/-   ##
==========================================
- Coverage   99.67%   99.58%   -0.09%     
==========================================
  Files         107      107              
  Lines       13245    13282      +37     
  Branches     1006     1013       +7     
==========================================
+ Hits        13202    13227      +25     
- Misses         28       35       +7     
- Partials       15       20       +5     

Impacted Files	Coverage Δ
trio/_core/__init__.py	100.00% <ø> (ø)
trio/lowlevel.py	100.00% <ø> (ø)
trio/_core/_ki.py	88.99% <87.35%> (-9.54%)	:arrow_down:
trio/_core/_run.py	99.73% <100.00%> (-0.01%)	:arrow_down:
trio/_threads.py	100.00% <100.00%> (ø)
trio/_tools/gen_exports.py	100.00% <100.00%> (ø)
trio/_core/tests/test_ki.py	99.70% <0.00%> (-0.30%)	:arrow_down:

[njsmith]

I like the overall approach. I wonder if we can simplify it though? In particular having 4 different protection states seems like a lot.

• "This and all its callees are protected" makes sense
• "This is protected but its callees aren't": I think this really only applies to contextlib and contextvars, so maybe we can just special-case those namespaces instead of tracking them via code objects?
• "Transparent": This is only for the benefit of currently_ki_protected, right, and only so it can give the right result when called from a "protected but its callees aren't" function? So (a) if we remove the general category of "protected but its callees aren't" then I think it's unnecessary, and (b) anyway, can't it just do sys._getframe(1) to test the caller's frame instead of its own?

The trickier one is "This and its callees are not protected". As noted in #733, I'm not convinced that @disable_ki_protection is really a feature we even want? Maybe instead what we want is a flag that says "if your stack walk reaches this frame, then stop and trust the contextvar instead"?

[oremanj]

"This is protected but its callees aren't": I think this really only applies to contextlib and contextvars, so maybe we can just special-case those namespaces instead of tracking them via code objects?

I also used it to remove one function call layer in _thread, but we could just put back the extra function call layer there.

"Transparent": This is only for the benefit of currently_ki_protected, right, and only so it can give the right result when called from a "protected but its callees aren't" function? So (a) if we remove the general category of "protected but its callees aren't" then I think it's unnecessary, and (b) anyway, can't it just do sys._getframe(1) to test the caller's frame instead of its own?

Agreed on both counts, except that I think we would need to apply this to signal_raise() and its callees also, if we want to be able to fully test the "protected but callees aren't" case. But if we get rid of that case, we get rid of the need for this one too.

The trickier one is "This and its callees are not protected". As noted in #733, I'm not convinced that @disable_ki_protection is really a feature we even want? Maybe instead what we want is a flag that says "if your stack walk reaches this frame, then stop and trust the contextvar instead"?

That's actually how it works here -- if you say @disable_ki_protection, then KI deliverability follows the contextvar. Probably the name should change at minimum.

More discussion in #733, which I'm working on writing up a response to now. :-)