with statements are not ensuring that __exit__ is called if __enter__ succeeds

@rhettinger

BPO	29988
Nosy	@rhettinger, @gpshead, @ncoghlan, @nirs, @njsmith, @markshannon, @embray, @serhiy-storchaka, @jdemeyer, @1st1, @ryanhiebert, @xgid, @jack1142
PRs	Protect C-implemented context managers, like lock, from ctrl-C. Fixes bpo-29988. #1799 bpo-29988: Only check evalbreaker after calls and on backwards egdes. #18334
Dependencies	bpo-31344: f_trace_opcodes frame attribute to switch to per-opcode tracing

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-12-10.17:37:50.631>
created_at = <Date 2017-04-04.23:42:57.982>
labels = ['interpreter-core', 'type-bug', '3.9', '3.10']
title = 'with statements are not ensuring that __exit__ is called if __enter__ succeeds'
updated_at = <Date 2021-12-10.17:37:50.630>
user = 'https://github.com/njsmith'

bugs.python.org fields:

activity = <Date 2021-12-10.17:37:50.630>
actor = 'Mark.Shannon'
assignee = 'none'
closed = True
closed_date = <Date 2021-12-10.17:37:50.631>
closer = 'Mark.Shannon'
components = ['Interpreter Core']
creation = <Date 2017-04-04.23:42:57.982>
creator = 'njs'
dependencies = ['31344']
files = []
hgrepos = []
issue_num = 29988
keywords = ['patch']
message_count = 60.0
messages = ['291148', '291157', '294272', '294296', '294400', '294402', '294404', '294434', '297177', '297179', '297180', '297182', '297185', '297186', '297189', '297190', '301235', '301248', '301274', '301277', '301278', '301285', '301289', '301360', '301429', '301553', '301557', '301566', '301573', '301601', '301609', '301610', '301611', '301620', '301630', '301660', '301667', '301668', '301672', '301675', '301682', '301698', '301717', '301721', '301771', '301869', '312874', '321225', '321318', '350123', '350134', '352026', '352039', '352199', '352212', '352457', '353415', '372499', '386711', '389922']
nosy_count = 15.0
nosy_names = ['rhettinger', 'gregory.p.smith', 'ncoghlan', 'nirs', 'njs', 'Mark.Shannon', 'erik.bray', 'serhiy.storchaka', 'jdemeyer', 'yselivanov', 'deleted0524', 'ryanhiebert', 'xgdomingo', 'jack1142', 'Ami Fischman']
pr_nums = ['1799', '18334']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue29988'
versions = ['Python 3.9', 'Python 3.10']

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 breaks for a with statement).

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:
    do_something_else()
try:
    ...
finally:
    ...

then (ignoring the issue about POP_TOP for a moment) the last instruction in *one* of these branches if followed immediately by SETUP_FINALLY, while in the other branch there's a JUMP_FORWARD, then the SETUP_FINALLY.

All the more reason for a more generic solution to this that doesn't depend strictly on the next op locally in the byte code.

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 doesn't really matter.

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 call __await__, and hence may run arbitrary Python code.

We can't see any immediate barriers to moving those calls up into BEFORE_ASYNC_WITH, such that what ends up on the eval loop's stack is the already resolved iterable for use by YIELD FROM, rather than combining GET_AWAITABLE+YIELD_FROM the way a normal await expression does.

That would then give the preamble:

BEFORE_ASYNC_WITH (resolves __aenter__ and __aexit__ to iterables)
LOAD_CONST 0
YIELD_FROM (need to skip signal processing here)
SETUP_ASYNC_WITH

And the postamble:

POP_BLOCK (need to skip signal processing until YIELD_FROM)
LOAD_CONST 0
WITH_CLEANUP_START
LOAD_CONST 0
YIELD_FROM
WITH_CLEANUP_FINISH

We also agree that adding some kind of test injection hook (potentially limited to pydebug builds, depending on exactly how it works) is likely to be a necessary to be able to test this.

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 before the with block instead of after as happens today. :/

See also https://www.python.org/dev/peps/pep-0492/#asynchronous-context-managers-and-async-with

Nick is actively working on making a test.

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 that runs the trace function after every opcode, not just every time the line number changes or we jump backwards in the bytecode.

bpo-31344 is a separate issue to add that underlying capability so we can write this test case.

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.

Also, to make this fully useful we need some way to protect arbitrary callables against interrupts anyway (to handle the case where the signal arrives during the actual __enter__ or __exit__ body), so saying that we also need to be able to protect __await__ isn't a big deal.

Right, if you look at the comments in the draft test case, we realised there are three things we currently need to protect:

POP_BLOCK -> WITH_CLEANUP_START (synchronous CM)
POP_BLOCK -> GET_AWAITABLE (asynchronous CM)
GET_AWAITABLE -> YIELD_FROM (asynchronous CM)

Now that I have a test case, I'm going to start looking at defining a new DEFER_PENDING_UNTIL opcode that skips pending call processing (and hence signals) until that particular opcode offset is reached.

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.

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.

If there are third-party libraries making heavy use of Py_AddPendingCall then it could be an issue, but any such libraries should already be making sure they never have more than one Py_AddPendingCall pending at a time, because you already have to assume that the processing might be arbitrarily delayed.

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 pending call" logic based on a frame attribute, but I'm going to try to replace that with a suitable call to _testcapi._pending_threadfunc, which would avoid the need to expose those details on the frame, and ensure that we're testing the logic that actually matters.

However, the good news is that:

the patch is complete enough that I think it demonstrates that this approach is potentially viable
because it only suspends pending call process in the *current* frame, it's safe to cross boundaries that may call arbitrary Python code (this turns out to be important for the async with case)

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 at the end of the trace function would trigger the just registered pending call (and the pending call processing isn't deferred in the trace function's frame).

Right now, the revised tests are failing though - I'm assuming that's a sign that the previous fix wasn't actually sufficient, and it only looked sufficient due to the way the errors were being emulated.

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 due to the lack of signal safety in the test CM itself.

For the asynchronous case, Greg and I aren't seeing any way to resolve the situation without somehow making the pending call processing event loop aware - otherwise even if the current frame postpones dealing with the pending call, it will still get processed as soon as YIELD_FROM returns control to the asyncio event loop. The one thing we think may work is to provide a way for asyncio (et al) to configure a thread such that all pending calls are routed to the thread's event loop for handling, rather than being processed directly by the eval loop.

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?

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 test case *pass*, as "async with" yields control back to the event loop at the YIELD_FROM points, and hence it's always going to be possible for signals to interrupt the transfer of control to and from __aenter__ or __aexit__, even if those methods are themselves implemented in C.

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 that last restriction is pretty silly, because what's the point of an async CM that never yields to the event loop. Right?

I see what you mean, but I think this patch is still useful even if it doesn't protect the event loop itself. To get the full benefit, you also need some way to protect your event loop from unexpected KeyboardInterrupt, but that's something Trio already solves and asyncio potentially could. And we knew from the start that to get the *most* benefit from this patch we would also need some way to protect arbitrary chunks of Python code from KeyboardInterrupt so you could protect the __(a)exit__ method itself; needing to protect the loop too isn't a huge stretch beyond that.

(It's possible that enough of uvloop is implemented in C to benefit from this?)

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 the default SIGINT handler with one that routes the signal to the event loop instead of trying to handle it in the interpreter's eval loop.

The latter approach does mean it will be more difficult to interrupt a runaway non-cooperative coroutine, but that's going to be a necessary trade-off in order to allow the event loop to reliably manage a graceful shutdown in the face of KeyboardInterrupt.

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 the time it raises KeyboardInterrupt directly. So we actually can have signal safety and deal with runaway coroutines at the same time, and this patch does provide a meaningful reduction in race conditions for trio [1]. In principle there's nothing stopping asyncio or other coroutine runners from implementing a similar strategy.

[1] actually this is currently a lie, because this patch reveals another independent race condition: there's no way for my clever signal handler to tell that it's in a sensitive function like trio.Lock.__aexit__ if it runs on the very first bytecode of the function, before the function has had a chance to mark itself as sensitive. But *that's* fixable with something like bpo-12857.

(tl;dr: this patch is more awesome than you realize, thanks for working on it :-))

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()
>>> def sigint_handler():
...     raise KeyboardInterrupt
... 
>>> loop.add_signal_handler(signal.SIGINT, sigint_handler)
>>> loop.run_forever()
Traceback (most recent call last):
    ...
KeyboardInterrupt

That way, dealing gracefully with KeyboardInterrupt is wholly under the event loop's control, rather than the event loop having to fight with the eval loop as to how Ctrl-C should be handled.

I've retitled this issue to specifically cover the synchronous signal-safe context management case and filed bpo-31387 to cover making it easy to switch asyncio over to cooperative SIGINT handling (rather than the default pre-emptive handling).

I've also filed bpo-31388 to cover providing APIs to defer signals and pending calls when running in the main thread.

Should I open a new issue for the __aexit__ part of this issue then, or...? The reason neither asyncio nor trio use the approach you describe is that it makes it impossible to break out of infinite loops, and trio in particular has entirely solved this problem modulo interpreter limitations like this one.

A new issue (depending on this one and potentially on bpo-31388) would be helpful, especially if you were able to use the testing trace hook from this PR to reproduce the problem.

The reason I've taken the async with change out of this issue is because it's untestable given the current state of asyncio - the pending call fires as soon as the YIELD_FROM opcode is reached and control returns to the asyncio event loop. If there's a simple(ish) coroutine driver we can add to manage KeyboardInterrupt adequately for testing purposes, I'd be prepared to do that, but I'd still like to consider the async case separately from the synchronous one.

Nathaniel, actually, I think bpo-31387 is the right one to comment on, as still being able to interrupt long-running loops in synchronous code is a quality of implementation concern for that RFE.

On another topic: you should add a test where the 'with' block exits by raising an exception, because in this case the interpreter jumps straight to WITH_CLEANUP_START, potentially skipping the DEFER_PENDING_UNTIL entirely.

In addition to adding a test case to ensure exceptions were handled correctly, I also added test cases for return, break, and continue, and all four new tests initially failed.

Adding a special case for WITH_CLEANUP_START resolved those, but the tests need an adjustment to account for the fact that some of the opcode offsets are unreachable in the test case that covers the early return.

a special case for WITH_CLEANUP_START

I guess this works for the sync case, but it seems kinda fragile, and obviously won't work for the async case. Do you think it's worth revisiting this idea from the OP?: "The eval loop's "check for signal handlers" code is run rarely, so we could afford to do relatively expensive things like check a lookaside table that says "no signal handlers when 13 < f_lasti <= 22"."

(Of course it should be 13 <= f_lasti < 22, apparently I was writing in a mirror or something.)

BTW, I looked at your branch, and figured out the problem with your async test. Asyncio isn't a problem (YIELD_FROM doesn't yield to the event loop, only an actual 'yield' statement does that); the problem was just that your test case's __aexit__ is written in Python, so you were exceptions delayed until the __aexit__ call, and then they were raised inside __aexit__ and the test couldn't detect that properly. I have a patch against 9b8891a that makes it pass. (It switches the pass condition to "either tracking_cm.enter_without_exit is False OR AsyncTrackingCM.__aexit__ is in the exception's traceback".) But of course it still has the problem with jumping past DEFER_PENDING_UNTIL. Do you want me to post the patch here, or in the other issue?

Yeah, I was already thinking the lookaside table might be a better idea (similar to co_lnotab replacing the old SET_LINENO opcodes), especially since that would also provide a quick way of introspecting code objects to see if they included any try/finally constructs. The special case was just a quick check to see if the problem was what I thought it was.

That's also a good point regarding __aexit__ (I figured out that was the problem in the sync case, but hadn't realised it also applied to the async case), so I'll restore those tests and opcode updates.

I'm also wondering how far we might be able to get by adjusting the pending signal processing such that we always execute at least one line from a function before checking for pending signals (that should be enough to protect the test's __aexit__, and it would also be enough to get inside a with statement that guarded a function body against signals).

I updated the PR at https://github.com/ncoghlan/cpython/pull/2/files with another attempt at reinstating the asynchronous CM test case that also tweaks the eval loop to ensure that the first few opcodes in a function are always executed before pending calls are checked.

This version highlights why I think asyncio really needs work before the async behaviour can be tested robustly in the standard library: it *hangs* somewhere in _asyncio.TaskStepMethWrapper once pending call handling is deferred long enough for the __aexit__ method to actually finish.

Here's the patch I mentioned: njsmith@62547dc

It has a crude but effective technique for handling the hanging issue :-). Alternatively one could use the World's Stupidest Coroutine Runner:

def run(coro):
    try:
        coro.send(None)
    except StopIteration:
        pass
    else:
        raise RuntimeError("yielded")

I'm also wondering how far we might be able to get by adjusting the pending signal processing such that we always execute at least one line from a function before checking for pending signals

Huh, that's a neat idea. I think it's defeated by 'while True: pass', though :-(. (You have to write it on two lines, but it compiles down to a single instruction that jumps to itself.)

After chatting to Facebook's Carl Shapiro here at the core dev sprint, I'm going to start exploring a hopefully more robust white-listing based approach where we check for pending calls in the following cases:

the instruction pointer either hasn't changed or has gone backwards and the instruction isn't a YIELD_FROM (the "return to the start of a loop" case)
the next instruction is RETURN_VALUE (the function postamble case)

For now, I'm going to *skip* checking for additional pending calls when exiting a frame via exception, based on the theory that if we're already handling an exception, it's OK to wait until something catches it before checking for potential additional exception sources.

I've pushed a variant that relies entirely on checking for instruction pointer changes, and it seems quite promising. However, a test_io failure introduced by this variant shows a new way for this version to *create* problems: because of the shift in the way signals are handled, the failing test switches from raising the exception as the last item inside the with statement to instead raising it as the first thing inside the __exit__ statement for a pure-Python context manager.

With bpo-17611 merged (which moves stack unwinding to the compiler), I expect the exact details of this problem to have changed, but the general problem still exists: Ctrl-C may lead to __exit__ (or __aexit__) not being called even after __enter__ (or __aenter__) returns successfully, and this may happen even for context managers implemented with uninterruptible methods (e.g. in C in CPython without calling back into any Python code).

The issue with synchronous 'with' can be solved by bpo-32949.

See also bpo-34066 for the problem with interruption before calling __enter__.

While Greg Smith and I both cringed at the idea when I first raised it, I'm becoming more and more convinced that the only way we're going to be able to make resource cleanup reliable is for with statements to have the ability to disable signal handling while __enter__ and __exit__ methods are running.

When a with statement switches signal handling off in a particular execution context, there'd then need to be some absolute timing deadline for switching them back on, so resource acquisition or cleanup that got stuck in an infinite loop could still be interrupted eventually.

If you combined that with the signal handling approach in https://github.com/ncoghlan/cpython/pull/2/files, then I think we'd have as solid a solution as CPython is likely to be able to provide.

Historically, in C, Python, and and low-level interrupt programming it was the responsibility of a person writing a signal handler to make minimal variable edits and not disrupt anything else in the ecosystem.

Are we changing that rule? (I'm still at the cringing stage and possibly haven't evolved my views yet).

The signal handler in this case is CPython's internal signal handling system thus any such onus falls on us...

The great signal handling hack of "set a flag that the interpreter loop checks on occasion" trick lasted a long time, but our VM has since evolved to be much more complicated than every bytecode being a safe interrupt point.

FYI - Most of the in-room conversation we tried to capture on this issue thread was at the core dev sprint a couple years ago (Sept 2017). It'll similarly take me a while to swap this context back in.

There is also the draft test case at ncoghlan#2

That needs updating to account for the eval loop changes since I last worked on it, though. (I think that conflict may also be keeping the bots from running, even after I updated the issue title to use the modern convention)

Is there a chance this will get resolved for 3.8? This is an important guarantee.

While there is technically still time, it'd take people spending dedicated time on it before the 3.8 release. I won't be.

Important? Sure. But not urgent; this bug is not new. We've lived with it for many releases already. I believe it has been around since 2.5 when with statements were introduced.

I expect to fix this is as part of the general improvements I am making to the bytecode, interpreter and compiler.
So it should be fixed for 3.9 but not for 3.8

It's also not unique to with statements - it applies to all finally clauses. The longstanding workaround when deterministic cleanup is absolutely critical has been to run the "real" application in a subthread, and devote the main thread to gracefully terminating the subthread when requested.

When cleanup is critical, but doing it in a deterministic order is less so, __del__ methods are often used to fill the gap (although they too can be interrupted by a subsequent Ctrl-C).

I also realized that allowing infinite loops in cleanup code to ignore Ctrl-C may actually be a tolerable outcome: in the worst case, users can still escalate to Ctrl-Break/kill -9/Force stop/etc and pull the entire OS process out from under the interpreter. It's not good, but may be worth it in order to better handle users pressing Ctrl-C multiple times.

As a note on the general pattern, a user at work diagnosed a ^C problem in their code when running on 2.7 to be due to Queue.get's

acquire()
try:
  ...
finally:
  release()

Pattern, with the KeyboardInterrupt triggering after acquire() but before the try is entered. so release() is never called.

A try finally pattern that probably alleviates this by entering the try block first might look like:

try:
acquire()
...
finally:
try:
release()
except ThreadError:
pass # interrupted before acquire() succeeded.

It'd be a shame if any with statements lock acquisition context managers need to be turned into that, but I _believe_ it'd be a viable workaround for the time being if this race is found to be biting anyone.

Belatedly clearing the issue assignment here - while I do still sometimes ponder this problem, I haven't been actively working on it since the 2017 core sprint where Greg & I made our last serious attempt at trying to improve the situation.

Mark's PR at #18334 looks very promising to me, though - my main request was just to bring over the tests I wrote at the 2017 core dev sprints, and confirm that the revised eval breaker logic solves the issue.

Does #1799 solve this issue
for synchronous with?

    with closing(this), closing(that):

If it does, can we backport this fix to python 3.6?

3.6 is used as system python for RHEL/Centos 8, will be used for at
least 5 years or so.

The bytecode instruction set has changed a lot since 3.6, so I think a backport would be impractical.

3.6 is in security fix only mode, so you'd need to take this up with Red Hat.

Navigation Menu

Repository navigation

with statements are not ensuring that exit is called if enter succeeds #74174

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

with statements are not ensuring that __exit__ is called if __enter__ succeeds #74174

Description

Activity

jdemeyer commented on Jun 28, 2017

embray commented on Jun 28, 2017

jdemeyer commented on Jun 28, 2017

ncoghlan commented on Sep 4, 2017

gpshead commented on Sep 4, 2017

ncoghlan commented on Sep 4, 2017

njsmith commented on Sep 4, 2017

ncoghlan commented on Sep 4, 2017

gpshead commented on Sep 4, 2017

njsmith commented on Sep 5, 2017

ncoghlan commented on Sep 5, 2017

ncoghlan commented on Sep 6, 2017

ncoghlan commented on Sep 7, 2017

njsmith commented on Sep 7, 2017

ncoghlan commented on Sep 7, 2017

njsmith commented on Sep 7, 2017

ncoghlan commented on Sep 7, 2017

njsmith commented on Sep 7, 2017

njsmith commented on Sep 7, 2017

ncoghlan commented on Sep 7, 2017

ncoghlan commented on Sep 7, 2017

ncoghlan commented on Sep 7, 2017

njsmith commented on Sep 8, 2017

ncoghlan commented on Sep 8, 2017

ncoghlan commented on Sep 8, 2017

njsmith commented on Sep 8, 2017

ncoghlan commented on Sep 8, 2017

njsmith commented on Sep 8, 2017

ncoghlan commented on Sep 8, 2017

ncoghlan commented on Sep 8, 2017

njsmith commented on Sep 8, 2017

ncoghlan commented on Sep 9, 2017

ncoghlan commented on Sep 11, 2017

ncoghlan commented on Feb 26, 2018

serhiy-storchaka commented on Jul 7, 2018

ncoghlan commented on Jul 9, 2018

rhettinger commented on Aug 21, 2019

gpshead commented on Aug 21, 2019

ncoghlan commented on Sep 11, 2019

rhettinger commented on Sep 12, 2019

gpshead commented on Sep 12, 2019

markshannon commented on Sep 12, 2019

ncoghlan commented on Sep 14, 2019

gpshead commented on Sep 27, 2019

ncoghlan commented on Jun 28, 2020

nirs commented on Feb 9, 2021

markshannon commented on Mar 31, 2021

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

with statements are not ensuring that exit is called if enter succeeds #74174