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

[Shu Ming]

于 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 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:
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 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')