classification
Title: 'with' loses ->bool exceptions
Type: behavior Stage: commit review
Components: Interpreter Core Versions: Python 3.0, Python 3.1, Python 2.7, Python 2.6, Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: amaury.forgeotdarc Nosy List: amaury.forgeotdarc, jyasskin, kevinwatters, loewis
Priority: normal Keywords: patch

Created on 2008-12-08 06:21 by jyasskin, last changed 2008-12-10 23:57 by amaury.forgeotdarc. This issue is now closed.

Files
File name Uploaded Description Edit
with_badbool.patch amaury.forgeotdarc, 2008-12-08 13:18
with_badbool_2.patch amaury.forgeotdarc, 2008-12-08 17:01
Messages (10)
msg77290 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-12-08 06:21
When a context manager's __exit__() method returns an object whose
conversion to bool raises an exception, 'with' loses that exception. For
example:

>>> class CM(object):
...   def __init__(self, exit_result):
...     self.exit_result = exit_result
...   def __enter__(self):
...     return 3
...   def __exit__(self, a, b, c):
...     return self.exit_result()
... 
>>> class TrueAsBool(object):
...   def __nonzero__(self): return True
... 
>>> class FalseAsBool(object):
...   def __nonzero__(self): return False
... 
>>> class FailAsBool(object):
...   def __nonzero__(self):
...     raise RuntimeError("Should see this but won't")
... 
>>> with CM(TrueAsBool):
...   raise AssertionError("Should NOT see this")
... 
>>> with CM(FalseAsBool):
...   raise AssertionError("Should see this")
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
AssertionError: Should see this
>>> with CM(FailAsBool):
...   raise AssertionError("Should see RuntimeException (oops)")
... 
>>> 


The problem is that WITH_CLEANUP only checks if PyObject_IsTrue(x)
returns non-zero, but that function returns <0 when the bool conversion
raises an exception.
msg77302 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-08 13:18
Good catch. Here is a patch, with test.
msg77324 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-12-08 16:40
I think your patch leaks a reference to 'x'. Move the Py_DECREF(x) to
before "if (err < 0)"?

And then really nitpicky: your test has 3 blank lines after it, and
should have 2.

Otherwise looks great. Thanks for picking this up!
msg77328 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-08 17:01
Running the test with -R:: indeed shows the reference leak.
2nd patch with suggested corrections.
msg77331 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-12-08 17:28
Looks good. Thanks!

I assume this should be merged into the 2.6.x and 3.0.x branches?
msg77394 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-12-09 08:29
The patch doesn't apply cleanly to the 2.5 branch, because that doesn't
have r61290. If somebody wants to backport it, go ahead (if you can do
so by Thursday).
msg77395 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-09 08:59
Does it need to be backported to 2.5? IMO it is a corner case.
msg77458 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-12-09 22:08
> Does it need to be backported to 2.5? IMO it is a corner case.

Not necessarily. If nobody volunteers, it won't be backported,
which is fine with me. The only potential release blocker for
a bug fix release can be a regression, which this isn't.
msg77484 - (view) Author: Jeffrey Yasskin (jyasskin) * (Python committer) Date: 2008-12-10 06:13
It's a corner case that would confuse anyone who ran into it, so I'm
happy to backport it. I'll probably just re-do your patch on the 2.5
branch since WITH_CLEANUP has a different structure there.
msg77578 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-10 23:57
Fixed with r67684 (2.5), r67688 (trunk), r67689 (py3k), r67690 (3.0), 
r67691 (2.6).
History
Date User Action Args
2008-12-10 23:57:38amaury.forgeotdarcsetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg77578
2008-12-10 06:13:27jyasskinsetmessages: + msg77484
2008-12-09 22:08:27loewissetmessages: + msg77458
2008-12-09 08:59:49amaury.forgeotdarcsetmessages: + msg77395
2008-12-09 08:29:46loewissetnosy: + loewis
messages: + msg77394
2008-12-08 19:01:01kevinwatterssetnosy: + kevinwatters
2008-12-08 17:28:26jyasskinsetmessages: + msg77331
2008-12-08 17:01:05amaury.forgeotdarcsetfiles: + with_badbool_2.patch
assignee: jyasskin -> amaury.forgeotdarc
messages: + msg77328
2008-12-08 16:40:18jyasskinsetkeywords: - needs review
resolution: accepted
messages: + msg77324
stage: patch review -> commit review
2008-12-08 13:18:11amaury.forgeotdarcsetfiles: + with_badbool.patch
nosy: + amaury.forgeotdarc
messages: + msg77302
priority: normal
keywords: + needs review, patch
stage: needs patch -> patch review
2008-12-08 06:21:09jyasskincreate