Thanks for your explanation and it is nice. I missed the for loop in the above and the code was trying to save the first exception from the multiple undo functions.on 2014/01/20 11:37, Shu Ming wrote:于 2014/1/16 13:17, Zhou Zheng Sheng 写道:According to Python documentation, context managers should not re-raise the exception passed to __exit__ method. If __exit__() does not return True, Python re-raises this exception automatically. Context managers should only raise exception from __exit__() itself. This means we should only re-raise exception caught from the undo functions. Another problem of the current implementation is that it suppresses str exceptions. This is because of bug in old Python version. When an exception is raised from within a "with" statement, exc_value would be a raw str or int rather than Exception instance, so when we re-raise the stored exc_value, it triggers another exception saying "exceptions must be old-style classes or derived from BaseException, not str". This patch fix this problem by raising the stored exception value along with its store exception type. http://docs.python.org/2/library/stdtypes.html#contextmanager.__exit__ Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- src/kimchi/rollbackcontext.py | 25 +++++------ tests/test_rollbackcontext.py | 102 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 13 deletions(-) create mode 100644 tests/test_rollbackcontext.py diff --git a/src/kimchi/rollbackcontext.py b/src/kimchi/rollbackcontext.py index 2afd114..9502169 100644 --- a/src/kimchi/rollbackcontext.py +++ b/src/kimchi/rollbackcontext.py @@ -45,24 +45,23 @@ class RollbackContext(object): return self def __exit__(self, exc_type, exc_value, traceback): - firstException = exc_value - + """ + According to Python official doc. This function should only re-raise + the exception from undo functions. Python automatically re-raises the + original exception when this function does not return True. + http://docs.python.org/2/library/stdtypes.html#contextmanager.__exit__ + """ + undoExcInfo = None for undo, args, kwargs in self._finally: try: undo(*args, **kwargs) - except Exception as e: + except Exception: # keep the earliest exception info - if not firstException: - firstException = e - # keep the original traceback info - traceback = sys.exc_info()[2] + if undoExcInfo is None:I think undoExcInfo is always None here. Why should we check it?Hi, this is because once we store the sys.exc_info() in undoExcInfo, it's not none anymore, and we just want to store the earliest exception we caught.
Here, your assumption is that return nothing will have the same effect as return false to the __exit()__ caller. I am not sure this will be true for ever and this really depends on Python implementation which may evolve in the future.+ undoExcInfo = sys.exc_info() - # re-raise the earliest exception - if firstException is not None: - if type(firstException) is str: - sys.stderr.write(firstException) - else: - raise firstException, None, traceback + if exc_type is None and undoExcInfo is not None: + raise undoExcInfo[0], undoExcInfo[1], undoExcInfo[2]Should we return a False or True for __exit()__ to make the context manager to re-raise or not?Thanks. There is not necessary to return False or True. Return True means we want to suppress the original exception, and we never does that. Return false is the same as return nothing.
Returning a true value from this method will cause the with statement to suppress the exception and continue execution with the statement immediately following the with statement. Otherwise the exception continues propagating after this method has finished executing. Exceptions that occur during execution of this method will replace any exception that occurred in the body of the with statement.
The exception passed in should never be reraised explicitly - instead, this method should return a false value to indicate that the method completed successfully and does not want to suppress the raised exception. This allows context management code (such as contextlib.nested) to easily detect whether or not an __exit__() method has actually failed.
There are 4 possible situations. 1. No exception raised from inside "with" statement, no exception raised from undo. 2. Exception raised from inside "with" statement, no exception raised from undo. 3. No exception raised from inside "with" statement, exception raised from undo. 4. Exception raised from inside "with" statement, exception raised from undo. As our purpose is to prefer the earliest exception, so every time there is an exception from inside "with", we should have the caller of __exit__ re-raise it. So in this case we can just ignore all exceptions raised from undo. This means for situation 2 and 3, we should just return. As regard to situation 1, just return is OK. For situation 3, the "if exc_type is None and undoExcInfo is not None" expression is a exactly check of situation 3. We should only re-raise exceptions from undo in this case.def defer(self, func, *args, **kwargs): self._finally.append((func, args, kwargs)) diff --git a/tests/test_rollbackcontext.py b/tests/test_rollbackcontext.py new file mode 100644 index 0000000..23b41c4 --- /dev/null +++ b/tests/test_rollbackcontext.py @@ -0,0 +1,102 @@ +# +# Project Kimchi +# +# Copyright IBM, Corp. 2013 +# +# Authors: +# Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +import unittest + +from kimchi.rollbackcontext import RollbackContext + + +class FirstError(Exception): + '''A hypothetical exception to be raise in the test firstly.''' + pass + + +class SecondError(Exception): + '''A hypothetical exception to be raise in the test secondly.''' + pass + + +class RollbackContextTests(unittest.TestCase): + + def setUp(self): + self._counter = 0 + + def _inc_counter(self): + self._counter += 1 + + def _raise(self, exception=FirstError): + raise exception() + + def test_rollback(self): + with RollbackContext() as rollback: + rollback.prependDefer(self._inc_counter) + rollback.prependDefer(self._inc_counter) + self.assertEquals(self._counter, 2) + + def test_raise(self): + try: + with RollbackContext() as rollback: + rollback.prependDefer(self._inc_counter) + rollback.prependDefer(self._inc_counter) + raise FirstError() + rollback.prependDefer(self._inc_counter) + except FirstError: + # All undo before the FirstError should be run + self.assertEquals(self._counter, 2) + else: + self.fail('Should have raised FirstError') + + def test_raise_undo(self): + try: + with RollbackContext() as rollback: + rollback.prependDefer(self._inc_counter) + rollback.prependDefer(self._raise) + rollback.prependDefer(self._inc_counter) + except FirstError: + # All undo should be run + self.assertEquals(self._counter, 2) + else: + self.fail('Should have raised FirstError') + + def test_raise_prefer_original(self): + try: + with RollbackContext() as rollback: + rollback.prependDefer(self._raise, SecondError) + raise FirstError() + except FirstError: + pass + except SecondError: + self.fail('Should have preferred FirstError to SecondError') + else: + self.fail('Should have raised FirstError') + + def test_raise_prefer_first_undo(self): + try: + with RollbackContext() as rollback: + rollback.prependDefer(self._raise, SecondError) + rollback.prependDefer(self._raise, FirstError) + except FirstError: + pass + except SecondError: + self.fail('Should have preferred FirstError to SecondError') + else: + self.fail('Should have raised FirstError')