[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: Attempting to clarify what Greg & I think the right answer will be for the async context management case: https://docs.python.org/3/library/asyncio-eventloop.html#unix-signals In practice, that would look something like: ``` >>> loop = asyncio.get_event_loop()

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nathaniel Smith added the comment: (tl;dr: this patch is more awesome than you realize, thanks for working on it :-)) -- ___ Python tracker ___

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nathaniel Smith added the comment: FWIW trio's strategy for handling this is to install a clever signal handle that routes the signal to the event loop IF the signal arrives while the event loop is running, or while particularly sensitive code like trio.Lock.__aexit__ is running. The rest of

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: I think you're agreeing with me - we can make synchronous context managers meaningfully more signal safe (at least for CMs implemented in C, or precompiled with Cython), but for asynchronous context managers, the only meaningful defense available is to replace

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nathaniel Smith added the comment: To make sure I understand correctly: your concern is that the event loop is not implemented in C. So if you have this patch + an async CM that's implemented in C + the async CM never *actually* yields to the event loop, then that will be signal safe... but

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: It isn't writing the test case that's the problem - the same technique we're using for the synchronous CM works for the asynchronous CM as well. The problem is that unlike the synchronous CM, the DEFER_PENDING_UNTIL opcode isn't sufficient to make the async CM

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nathaniel Smith added the comment: For purposes of writing a test case, can you install a custom Python-level signal handler, and make some assertion about where it runs? I.e., the calling frame should be inside the __aexit__ body, not anywhere earlier? --

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: The updated PR fully resolves the synchronous CM case by switching to using threading.Lock as the test CM (as per Nathaniel's original suggestion). Without that, the pending exception was being processed as soon as __exit__() started running, so the test failed

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: I've updated the draft PR to actually raise the exception from a pending call hook, rather than emulating it in a trace hook. This was a bit trickier than I first expected, as it required moving the trace hook itself into C, as otherwise the "return" bytecode

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: https://github.com/ncoghlan/cpython/pull/2/files now adds a DEFER_PENDING_UNTIL opcode that relies on the jump offset calculation machinery to decide how long pending call processing should be deferred. Right now, the test case is also emulating the "skip

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nathaniel Smith added the comment: In bpo-30703 Antoine fixed signal handling so it doesn't use Py_AddPendingCall anymore. At this point the only time the interpreter itself uses Py_AddPendingCall is when there's an error writing to the signal_fd, which should never happen in normal usage.

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Gregory P. Smith added the comment: I do wonder if this is going to raise the urgency of https://bugs.python.org/issue16565 (Increase Py_AddPendingCall array size) or other refactoring of how pending calls are tracked. -- ___ Python tracker

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: Right, if you look at the comments in the draft test case, we realised there are three things we currently need to protect: 1. POP_BLOCK -> WITH_CLEANUP_START (synchronous CM) 2. POP_BLOCK -> GET_AWAITABLE (asynchronous CM) 3. GET_AWAITABLE -> YIELD_FROM

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nathaniel Smith added the comment: This would still provide value even if you have to do a GET_AWAITABLE in the protected region: the most common case is that __aenter__ is a coroutine function, which means that its __await__ is implemented in C and already protected against interrupts.

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Changes by Nick Coghlan : -- dependencies: +f_trace_opcodes frame attribute to switch to per-opcode tracing ___ Python tracker ___

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: https://github.com/ncoghlan/cpython/pull/2/files provides a test case that reliably reproduces the problem for both synchronous and asynchronous context managers. It's inspired by Nathaniel's proposal above, but relies on a modified version of sys.settrace

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Gregory P. Smith added the comment: Yury's in the room here and pointed out how Nick and I are wrong. :) [yay sprints!] async def __aexit__(self, *e): spamity_spam return Awaitable If we resolve the GET_AWAITABLE at BEFORE_ASYNC_WITH time, the spamity_spam within __aexit__ is executed

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: Greg Smith & I are looking at this at the core dev sprint, and we think some variant of the "atomic until" idea should work, but there's a prerequisite change to the way "async with" works: the "GET_AWAITABLE" opcodes need to be avoided in this case, as they

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Changes by Gregory P. Smith : -- nosy: +gregory.p.smith ___ Python tracker ___ ___

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Changes by Xavier G. Domingo : -- nosy: +xgdomingo ___ Python tracker ___ ___

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Jeroen Demeyer added the comment: > Actually, I think it's between the end of the `try` and the beginning of the > `finally` (which is precisely the same place that *breaks* for a with > statement). Sorry, this is wrong and Erik was right. But the special case doesn't work anyway, so it

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Erik Bray added the comment: Actually I just finished reading njs's blog post, and as he points out that special case for SETUP_FINALLY is basically broken. There are other cases where it doesn't really work either. For example if you have: if ...: do_something() else:

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Jeroen Demeyer added the comment: > It seems like that does at least try to guarantee that a signal can't > interrupt between: > > lock.acquire() > try: > ... Actually, I think it's between the end of the `try` and the beginning of the `finally` (which is precisely the same place that

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Erik Bray added the comment: >> Or we could steal a bit in the opcode encoding or something. > That seems like a very reasonable and easy-to-implement solution. It > would > generalize this check: >

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Jeroen Demeyer added the comment: > Or we could steal a bit in the opcode encoding or something. That seems like a very reasonable and easy-to-implement solution. It would generalize this check:

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: Sorry, I misread Nathaniel's suggestion - the exhaustive search of all possible offsets already avoids the need to hardcode any code generation dependent details in the tests. So the trace function approach would just avoid the need for that search. --

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: One testing possibility worth considering: perhaps it would be make sense to have "start_try" and "start_finally" trace events, such that a trace function could be installed that injected signals at the worst possible time in the opcode evaluation? I haven't

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Jeroen Demeyer added the comment: Nice analysis. I always assumed that `with` was safe from such race conditions, but it isn't. -- ___ Python tracker

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Changes by Jeroen Demeyer : -- nosy: +jdemeyer ___ Python tracker ___ ___

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nathaniel Smith added the comment: On further thought, I think the way I'd write a test for this is: (1) add a testing primitive that waits for N instructions and then injects a SIGINT. Probably this would require tweaking the definition of Py_MakePendingCalls like I described in my previous

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Changes by Mark Shannon : -- pull_requests: +1882 ___ Python tracker ___ ___ Python-bugs-list

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nathaniel Smith added the comment: > If all you need is that with foo: pass guarantees that either both or neither > of __enter__ and __exit__ are called, for C context managers, and only C > context managers, then the fix is trivial. It would be nice to have it for 'async with foo: pass' as

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Mark Shannon added the comment: Nathaniel, Do you have any way to reliably test for this failure mode? -- ___ Python tracker ___

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Mark Shannon added the comment: If all you need is that with foo: pass guarantees that either both or neither of __enter__ and __exit__ are called, for C context managers, and only C context managers, then the fix is trivial. To protect Python code would need a custom context manager

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nathaniel Smith added the comment: Right, fixing this bug alone can't make programs control-C safe in general. But there exist techniques to make __enter__/__exit__ methods safe WRT control-C, and if we fix this bug *and* apply those techniques then you can get some meaningful guarantees.

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Mark Shannon added the comment: This is either a "won't fix" or an "impossible to fix" depending on your point of view. PATIENT: It hurts whenever I press ctrl-C DOCTOR: Then don't press ctrl-C The problem is that ctrl-C can provoke an interrupt at any point in the program, and

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

Nick Coghlan added the comment: My first thought would be to inject a wordcode instruction that's essentially an eval loop directive: "ATOMIC_UNTIL " ATOMIC_UNTIL would have at least the following properties: - checks for signals are skipped until the given offset is reached - checks to

[issue29988] (async) with blocks and try/finally are not as KeyboardInterrupt-safe as one might like

New submission from Nathaniel Smith: You might hope the interpreter would enforce the invariant that for 'with' and 'async with' blocks, either '__(a)enter__' and '__(a)exit__' are both called, or else neither of them is called. But it turns out that this is not true once KeyboardInterrupt