classification
Title: Exception context is not suppressed correctly in contextlib.ExitStack.__exit__
Type: behavior Stage: resolved
Components: Interpreter Core, Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: alonho, ncoghlan, python-dev
Priority: normal Keywords: patch

Created on 2012-05-31 13:41 by ncoghlan, last changed 2012-06-01 12:48 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
14963.3.patch alonho, 2012-06-01 08:09 review
Messages (6)
msg161998 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-05-31 13:41
When adding the test case for #14963, I discovered that contextlib.ExitStack can't *quite* reproduce the exception handling of nested with statements.

The problem arises when the original exception gets suppressed by one of the handlers, but an outer handler raises a *new* exception, then nested with statements will correctly indicate that there was no exception context active when the new exception was raised (since the inner with statement will fully clear the exception state).

By contrast, when using ExitStack, the interpreter will add the original exception from inside the body of the with statement as the context for the *new* exception, even though the inner exception had been suppressed before the outer one was encountered.

Restoring sys.exc_clear() *might* allow this discrepancy to be resolved by explicitly clearing the exception state when one of the callbacks indicates that the current exception has been handled (although it might be trickier than that, if the problem is actually due to caching the exception state inside the with cleanup code in the eval loop)
msg162021 - (view) Author: alon horev (alonho) * Date: 2012-05-31 21:23
Another possible solution is to explicitly set an exception's __supress_context__ attribute to False (right now impossible because it's the default value). 
If a user can 'turn on' the flag when attaching a different exception (raise X from Y), why not allow 'turning it off'? (symmetry anyone?) right now it is set to False by default and changed to true when 'raising from'.
I suggest changing the default to None, allowing the user to explicitly say: I'm no longer in the previous exception's context.

Feels a bit like solving our hack with another hack (:

And about the PSF contrib agreement, I'll do it as soon as I'm near a printer. too bad we're using pens and not RSA private keys for signatures (-:

   thanks, Alon
msg162037 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-06-01 01:11
Electronic contributor forms are actually on the PSF wishlist. We'll get there some day (maybe after the website update).

Getting back to the problem at hand, I think you might be on to something with the idea of exploiting PEP 409 to handle this. Specifically, where we reraise a caught exception when there are no exception details active, we should be able to replace the bare raise with something like:

    # A containing with statement will automatically add the exception
    # context back in after it gets suppressed. Avoid displaying it.
    if suppressed_exc and exc_details == (None, None, None):
        raise exc from None
    raise

That way, the exception *display* of escaping exceptions will still match that of nested with statements, even though the attributes are subtly different (__suppress_context__ being set to True, rather than __context__ being None)
msg162055 - (view) Author: alon horev (alonho) * Date: 2012-06-01 08:09
Ok, so turns out this was just a stupid bug: we set the __context__ attr only if an exception is raised, but not when an exception has been previously 'cleared'. so the context is filled (by python) with the last exception raised which is the outer one.
deleting the 'if last context is an exception' solved it.

This is how I understood it:
the exception's __context__ is set when it's raised and not in its except clause, meaning there is no way the outer with is mutating our inner exceptions. using pdb I saw the outer exception being explicitly set.
msg162072 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-06-01 12:38
Ah, you're absolutely right. Interestingly though, the old recursive implementation appeared to have the same bug, and I'd deliberately used the recursive approach precisely so that suppressed exceptions *would* get cleared correctly. I must have managed to leave a loophole somewhere :(

I also realised simply overwriting the context isn't correct in all cases, and have adjusted the test cases accordingly.
msg162073 - (view) Author: Roundup Robot (python-dev) Date: 2012-06-01 12:48
New changeset c108bc96aec6 by Nick Coghlan in branch 'default':
Close #14969: Improve the handling of exception chaining in contextlib.ExitStack
http://hg.python.org/cpython/rev/c108bc96aec6
History
Date User Action Args
2012-06-01 12:48:50python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg162073

resolution: fixed
stage: needs patch -> resolved
2012-06-01 12:38:30ncoghlansettype: enhancement -> behavior
messages: + msg162072
title: Use __suppress_context__ in contextlib.ExitStack.__exit__ -> Exception context is not suppressed correctly in contextlib.ExitStack.__exit__
2012-06-01 08:09:41alonhosetfiles: + 14963.3.patch
keywords: + patch
messages: + msg162055
2012-06-01 01:11:32ncoghlansetmessages: + msg162037
title: Restore sys.exc_clear()? -> Use __suppress_context__ in contextlib.ExitStack.__exit__
2012-05-31 21:23:14alonhosetnosy: + alonho
messages: + msg162021
2012-05-31 13:41:44ncoghlancreate