[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Senthil Kumaran added the comment: The last tracker message msg207926 is applicable to issue #19097 and not here. Sorry for the confusion. -- nosy: +orsenthil ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Roundup Robot added the comment: New changeset a3e49868cfd0 by Senthil Kumaran in branch '3.3': Issue #19092 - Raise a correct exception when cgi.FieldStorage is given an http://hg.python.org/cpython/rev/a3e49868cfd0 New changeset 1638360eea41 by Senthil Kumaran in branch 'default': merge from 3.3 http://hg.python.org/cpython/rev/1638360eea41 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Changes by Nick Coghlan ncogh...@gmail.com: -- assignee: - ncoghlan ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Roundup Robot added the comment: New changeset 423736775f6b by Nick Coghlan in branch '3.3': Close #19092: ExitStack now reraises exceptions from __exit__ http://hg.python.org/cpython/rev/423736775f6b New changeset 451f5f6151f5 by Nick Coghlan in branch 'default': Merge #19092 from 3.3 http://hg.python.org/cpython/rev/451f5f6151f5 -- nosy: +python-dev resolution: - fixed stage: - committed/rejected status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Hrvoje Nikšić added the comment: Indeed, that works, thanks. Here is the updated patch for review, passing all tests. -- Added file: http://bugs.python.org/file31908/exitstack.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Hrvoje Nikšić added the comment: Here is the updated patch, with a very minor improvement (no longer unnecessarily holds on to original exc_info), and with new tests. The tests test for the non-suppression of exit-exception (which fails without the fix) and for the correct suppression of body-exception by an outer CM. It was not necessary to write a test for suppression of exit-exception, since this is already tested by test_exit_exception_chaining_suppress(). There is one problem, however: applying my patch mysteriously breaks the existing test_exit_exception_chaining(). It breaks in a peculiar way: the correct exception is propagated , but the exception's context is wrong. Instead of to KeyError, it points to ZeroDivisionError, despite its having been correctly suppressed. I placed prints in _fix_exception_context right before assignment to __context__, to make sure it wasn't broken by my patch, and the assignment appears correct, it sets the context of IndexError to KeyError instance. The __context__ is correct immediately before the raise statement. However, after the IndexError is caught inside test_exit_exception_chaning(), its __context__ is unexpectedly pointing to ZeroDivisionError. It would seem that the difference is in the raise syntax. The old code used raise, while the new code uses raise exc[1], which I assume changes the exception's __context__. Changing raise exc[1] to raise exc[1] from None didn't help. -- Added file: http://bugs.python.org/file31897/exitstack.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Nick Coghlan added the comment: Moving the context fixing into an exception handler may work. Something like: try: raise exc[1] except BaseException as fix_exc: ... raise -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Hrvoje Nikšić added the comment: Nick, thanks for the review. Do you need me to write the patch for the test suite along with the original patch? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Nick Coghlan added the comment: That would be very helpful! -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
New submission from Hrvoje Nikšić: While using contextlib.ExitStack in our project, we noticed that its __exit__ method of contextlib.ExitStack suppresses the exception raised in any contextmanager's __exit__ except the outermost one. Here is a test case to reproduce the problem: class Err: def __enter__(self): pass def __exit__(self, *exc): 1/0 class Ok: def __enter__(self): pass def __exit__(self, *exc): pass import contextlib s = contextlib.ExitStack() s.push(Ok()) s.push(Err()) with s: pass Since the inner context manager raises in __exit__ and neither context manager requests suppression, I would expect to see a ZeroDivisionError raised. What actually happens is that the exception is suppressed. This behavior caused us quite a few headaches before we figured out why we the exceptions raised in during __exit__ went silently undetected in production. The problem is in ExitStack.__exit__, which explicitly propagates the exception only if it occurs in the outermost exit callback. The idea behind it appears to be to simply record the raised exception in order to allow exit callbacks of the outer context managers to see the it -- and get a chance to suppress it. The only exception that is directly re-raised is the one occurring in the outermost exit callback, because it certainly cannot be seen nor suppressed by anyone else. But this reasoning is flawed because if an exception happens in an inner cm and none of the outer cm's chooses to suppress it, then there will be no one left to raise it. Simply returning True from ExitStack.__exit__ is of no help, as that only has an effect when an exception was passed into the function in the first place, and even then, the caller can only re-raise the earlier exception, not the exception that occurred in the exit callback. And if no exception was sent to ExitStack.__exit__, as is the case in the above code, then no exception will be re-raised at all, effectively suppressing it. I believe the correct way to handle this is by keeping track of whether an exception actually occurred in one of the _exit_callbacks. As before, the exception is forwarded to next cm's exit callbacks, but if none of them suppresses it, then the exception is re-raised instead of returning from the function. I am attaching a patch to contextlib.py that implements this change. The patch also makes sure that True is returned from ExitStack.__exit__ only if an exception was actually passed into it. -- components: Library (Lib) files: exitstack.diff keywords: patch messages: 198411 nosy: hniksic priority: normal severity: normal status: open title: ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers type: behavior versions: Python 3.4 Added file: http://bugs.python.org/file31872/exitstack.diff ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Changes by Eric Snow ericsnowcurren...@gmail.com: -- nosy: +ncoghlan ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Changes by Barry A. Warsaw ba...@python.org: -- nosy: +barry ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue19092] ExitStack.__exit__ incorrectly suppresses exceptions in __exit__ callbacks of inner context managers
Nick Coghlan added the comment: Yep, as indicated by the patch, looks like just a bug with the location of the raise in the stack emulation. The contextlib tests will also need a new test case to cover this, as well as one to cover such an exception being suppressed by an outer manager. -- versions: +Python 3.3 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue19092 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
