[Kimchi-devel] [PATCH] RollbackContext: Make it complied with Python Standard

[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 at 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:
+                    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/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 at 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')
-- 
1.7.11.7