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

[Mark Shannon]

Change by Mark Shannon :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Mark Shannon]

Mark Shannon  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nir Soffer]

Nir Soffer  added the comment:

Does https://github.com/python/cpython/pull/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.

--
nosy: +nirs

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Gregory P. Smith]

Change by Gregory P. Smith :


--
versions: +Python 3.10 -Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[jack1142]

Change by jack1142 :


--
nosy: +jack1142

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

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 https://github.com/python/cpython/pull/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.

--
assignee: ncoghlan -> 

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Mark Shannon]

Change by Mark Shannon :


--
keywords: +patch
pull_requests: +17706
stage: test needed -> patch review
pull_request: https://github.com/python/cpython/pull/18334

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ami Fischman]

Change by Ami Fischman :


--
nosy: +Ami Fischman

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Mark Shannon]

Mark Shannon  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

Raymond Hettinger  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

There is also the draft test case at https://github.com/ncoghlan/cpython/pull/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)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Ryan Hiebert]

Change by Ryan Hiebert :


--
nosy: +ryanhiebert

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Raymond Hettinger]

Raymond Hettinger  added the comment:

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).

--
nosy: +rhettinger

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Gregory P. Smith]

Change by Gregory P. Smith :


--
versions: +Python 3.9 -Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Gregory P. Smith]

Change by Gregory P. Smith :


--
versions: +Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

The issue with synchronous 'with' can be solved by issue32949.

See also issue34066 for the problem with interruption before calling __enter__.

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan  added the comment:

With issue 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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

Here's the patch I mentioned: 
https://github.com/njsmith/cpython/commit/62547dc5ea323a07c25c2047636a02241f518013

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.)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

> 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 9b8891a7ca5 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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

Nathaniel, actually, I think issue 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

A new issue (depending on this one and potentially on issue 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nathaniel Smith]

Nathaniel Smith added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

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

[Nick Coghlan]

Nick Coghlan added the comment:

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

--
title: (async) with blocks and try/finally are not as KeyboardInterrupt-safe as 
one might like -> with statements are not ensuring that __exit__ is called if 
__enter__ succeeds

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com