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

于 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(a)linux.vnet.ibm.com&gt; > --- > 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?

> + 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?

> > 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(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')