<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">于 2014/1/20 14:38, Zhou Zheng Sheng 写道:<br>
</div>
<blockquote cite="mid:52DCC44B.3020304@linux.vnet.ibm.com"
type="cite">
<pre wrap="">on 2014/01/20 11:37, Shu Ming wrote:
</pre>
<blockquote type="cite">
<pre wrap="">于 2014/1/16 13:17, Zhou Zheng Sheng 写道:
</pre>
<blockquote type="cite">
<pre wrap="">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.
<a class="moz-txt-link-freetext" href="http://docs.python.org/2/library/stdtypes.html#contextmanager.__exit__">http://docs.python.org/2/library/stdtypes.html#contextmanager.__exit__</a>
Signed-off-by: Zhou Zheng Sheng <a class="moz-txt-link-rfc2396E" href="mailto:zhshzhou@linux.vnet.ibm.com"><zhshzhou@linux.vnet.ibm.com></a>
---
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.
+
<a class="moz-txt-link-freetext" href="http://docs.python.org/2/library/stdtypes.html#contextmanager.__exit__">http://docs.python.org/2/library/stdtypes.html#contextmanager.__exit__</a>
+ """
+ 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:
</pre>
</blockquote>
<pre wrap="">I think undoExcInfo is always None here. Why should we check it?
</pre>
</blockquote>
<pre wrap="">
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.</pre>
</blockquote>
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. <br>
<br>
<blockquote cite="mid:52DCC44B.3020304@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<blockquote type="cite">
<pre wrap="">+ 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]
</pre>
</blockquote>
<pre wrap="">
Should we return a False or True for __exit()__ to make the context
manager to re-raise or not?
</pre>
</blockquote>
<pre wrap="">
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.</pre>
</blockquote>
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. <br>
<br>
I excerpt some lines from python docs. The word in bold tell us to
return a false in such a case. <br>
---<br>
<p>Returning a true value from this method will cause the <a
class="reference internal"
href="http://docs.python.org/3/reference/compound_stmts.html#with"><tt
class="xref std std-keyword docutils literal"><span
class="pre">with</span></tt></a> statement
to suppress the exception and continue execution with the
statement immediately
following the <a class="reference internal"
href="http://docs.python.org/3/reference/compound_stmts.html#with"><tt
class="xref std std-keyword docutils literal"><span
class="pre">with</span></tt></a> 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 <a class="reference internal"
href="http://docs.python.org/3/reference/compound_stmts.html#with"><tt
class="xref std std-keyword docutils literal"><span
class="pre">with</span></tt></a> statement.</p>
<p><b>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 </b><b><tt class="docutils
literal"><span class="pre">contextlib.nested</span></tt></b><b>)
to easily detect whether
or not an </b><b><a class="reference internal"
href="http://docs.python.org/3/library/stdtypes.html?highlight=contextmanager.__exit__#contextmanager.__exit__"
title="contextmanager.__exit__"><tt class="xref py py-meth
docutils literal"><span class="pre">__exit__()</span></tt></a></b><b>
method has actually failed.</b></p>
<br>
<br>
<blockquote cite="mid:52DCC44B.3020304@linux.vnet.ibm.com"
type="cite">
<pre wrap="">
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.
</pre>
<blockquote type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">
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 <a class="moz-txt-link-rfc2396E" href="mailto:zhshzhou@linux.vnet.ibm.com"><zhshzhou@linux.vnet.ibm.com></a>
+#
+# 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')
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<br>
</body>
</html>