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. This patch also adds unit test for RollbackContext. http://docs.python.org/2/library/stdtypes.html#contextmanager.__exit__ v1 -> v2 Add tests/test_rollbackcontext.py to tests/Makefile.am. Add tests/test_rollbackcontext.py to Makefile.am PEP8 list. Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; --- Makefile.am | 1 + src/kimchi/rollbackcontext.py | 26 +++++------ tests/Makefile.am | 1 + tests/test_rollbackcontext.py | 102 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 13 deletions(-) create mode 100644 tests/test_rollbackcontext.py diff --git a/Makefile.am b/Makefile.am index 04ad696..b09a545 100644 --- a/Makefile.am +++ b/Makefile.am @@ -60,6 +60,7 @@ PEP8_WHITELIST = \ tests/test_model.py \ tests/test_plugin.py \ tests/test_rest.py \ + tests/test_rollbackcontext.py \ tests/test_storagepool.py \ tests/utils.py \ $(NULL) diff --git a/src/kimchi/rollbackcontext.py b/src/kimchi/rollbackcontext.py index 2afd114..d493af2 100644 --- a/src/kimchi/rollbackcontext.py +++ b/src/kimchi/rollbackcontext.py @@ -6,6 +6,7 @@ # Authors: # Adam Litke <agl(a)linux.vnet.ibm.com&gt; # ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; +# Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -45,24 +46,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: + 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] def defer(self, func, *args, **kwargs): self._finally.append((func, args, kwargs)) diff --git a/tests/Makefile.am b/tests/Makefile.am index 55d3ab4..59e344d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -31,6 +31,7 @@ EXTRA_DIST = \ test_osinfo.py \ test_plugin.py \ test_rest.py \ + test_rollbackcontext.py \ test_server.py \ test_storagepool.py \ test_vmtemplate.py \ diff --git a/tests/test_rollbackcontext.py b/tests/test_rollbackcontext.py new file mode 100644 index 0000000..fea430d --- /dev/null +++ b/tests/test_rollbackcontext.py @@ -0,0 +1,102 @@ +# +# Project Kimchi +# +# Copyright IBM, Corp. 2014 +# +# Authors: +# Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; +# +# 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')

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. This patch also adds unit test for RollbackContext. http://docs.python.org/2/library/stdtypes.html#contextmanager.__exit__ v1 -> v2 Add tests/test_rollbackcontext.py to tests/Makefile.am. Add tests/test_rollbackcontext.py to Makefile.am PEP8 list. Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; --- Makefile.am | 1 + src/kimchi/rollbackcontext.py | 26 +++++------ tests/Makefile.am | 1 + tests/test_rollbackcontext.py | 102 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 13 deletions(-) create mode 100644 tests/test_rollbackcontext.py diff --git a/Makefile.am b/Makefile.am index 04ad696..b09a545 100644 --- a/Makefile.am +++ b/Makefile.am @@ -60,6 +60,7 @@ PEP8_WHITELIST = \ tests/test_model.py \ tests/test_plugin.py \ tests/test_rest.py \ + tests/test_rollbackcontext.py \ tests/test_storagepool.py \ tests/utils.py \ $(NULL) diff --git a/src/kimchi/rollbackcontext.py b/src/kimchi/rollbackcontext.py index 2afd114..d493af2 100644 --- a/src/kimchi/rollbackcontext.py +++ b/src/kimchi/rollbackcontext.py @@ -6,6 +6,7 @@ # Authors: # Adam Litke <agl(a)linux.vnet.ibm.com&gt; # ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; +# Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -45,24 +46,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: + 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] def defer(self, func, *args, **kwargs): self._finally.append((func, args, kwargs)) diff --git a/tests/Makefile.am b/tests/Makefile.am index 55d3ab4..59e344d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -31,6 +31,7 @@ EXTRA_DIST = \ test_osinfo.py \ test_plugin.py \ test_rest.py \ + test_rollbackcontext.py \ test_server.py \ test_storagepool.py \ test_vmtemplate.py \ diff --git a/tests/test_rollbackcontext.py b/tests/test_rollbackcontext.py new file mode 100644 index 0000000..fea430d --- /dev/null +++ b/tests/test_rollbackcontext.py @@ -0,0 +1,102 @@ +# +# Project Kimchi +# +# Copyright IBM, Corp. 2014 +# +# Authors: +# Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; +# +# 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')