Protect C-implemented context managers, like lock, from ctrl-C. Fixes bpo-29988.

[markshannon]

Prevents Ctrl-C interrupting between the POP_BLOCK and END_WITH in the following bytecode sequence:

SETUP_WITH
....
POP_BLOCK
LOAD_CONST None
END_WITH

[mention-bot]

@markshannon, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @serhiy-storchaka and @1st1 to be potential reviewers.

[1st1]

I think one of the issues is that WITH_CLEANUP opcode was split into WITH_CLEANUP_START and WITH_CLEANUP_FINISH in CPython 3.5. For synchronous with, the opcodes will be compiled to come one after another. And asynchronous async with will be compiled to something like this:

WITH_CLEANUP_START
GET_AWAITABLE
LOAD_CONST (None)
YIELD_FROM
WITH_CLEANUP_FINISH

The thing that @njsmith wants to fix is that the KeyboardInterrupt might happen right after GET_AWAITABLE (for instance), which breaks the async context manager protocol.

I'm not sure that without some new mechanism to guarantee that KeyboardInterrupt cannot be delivered between WITH_CLEANUP_START and WITH_CLEANUP_FINISH, both sync and async context managers are free of this problem.

[njsmith]

The removal of the SETUP_FINALLY special case seems reasonable to me on its own merits, but I don't see how it relates to the rest of the PR/bug report?

The use of FAST_DISPATCH after POP_BLOCK does AFAICT fix the with case, at least as long as the bytecode compiler continues to produce the same output. Though I suspect it will break again sooner or later and no-one will notice because there's no test. And it doesn't fix the more difficult async with case.

@1st1: I think the WITH_CLEANUP_START / WITH_CLEANUP_FINISH split is not a problem for plain with? WITH_CLEANUP_START is both the target of the finally jump and also runs the exit_func, so even if a signal arrives after WITH_CLEANUP_START then we'll still have released the lock or whatever. (I guess WITH_CLEANUP_END and END_FINALLY do manipulate the exception state so it's possible that a KeyboardInterrupt at that stage might end up with incorrect implicit exception chaining or something? I haven't read them carefully.) Of course async with is more complicated, since WITH_CLEANUP_START only fetches the awaitable object, and it takes several more bytecodes to execute it and actually release the lock (or whatever).

[serhiy-storchaka]

It would be better to discuss the design on the bugtracker.

[njsmith]

Though I suspect it will break again sooner or later and no-one will notice because there's no test

I added a comment on the bugtracker describing one approach we could use to write a robust test for this.

[ncoghlan]

I'm going to close this proposed update - as per the discussion on the tracker, I think the more robust solution is going to be to switch to being more selective in general about where we allow pending calls and asynchronous exceptions to be handled.