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

Shu Ming shuming at linux.vnet.ibm.com
Tue Jan 21 00:59:51 UTC 2014


于 2014/1/20 16:24, Zhou Zheng Sheng 写道:
> on 2014/01/20 15:51, Shu Ming wrote:
>> 于 2014/1/20 14:38, Zhou Zheng Sheng 写道:
>>> on 2014/01/20 11:37, Shu Ming wrote:
>>>> 于 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?
>>>>
>>> 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.
>> 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.
>>
>>>>> +                    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?
>>>>
>>> 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.
>> 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.
>>
>> I excerpt some lines from python docs.  The word in bold tell us to
>> return a false in such a case.
>> ---
>>
>> Returning a true value from this method will cause the with
>> <http://docs.python.org/3/reference/compound_stmts.html#with> statement
>> to suppress the exception and continue execution with the statement
>> immediately following the with
>> <http://docs.python.org/3/reference/compound_stmts.html#with> 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 with
>> <http://docs.python.org/3/reference/compound_stmts.html#with> statement.
>>
>> *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
>> **contextlib.nested**) to easily detect whether or not an **__exit__()
>> <http://docs.python.org/3/library/stdtypes.html?highlight=contextmanager.__exit__#contextmanager.__exit__>**method
>> has actually failed.*
>>
> Thanks for the detail. Would you please have a look at the following link?
> http://www.python.org/dev/peps/pep-0343/
>
> This is the actual standard of the "with" statement and context
> managers. The author is Guido van Rossum. You can see it says the following.
>
> "The motivation for this detail is to make it possible for
> mgr.__exit__() to swallow exceptions, without making it too easy (since
> the default return value, None, is false and this causes the exception
> to be re-raised)."
>
> The above sentence implies that the caller should treat "None" returned
> by __exit__ the same as "False".
>
> You may also notice that in this PEP, all __exit__ examples actually do
> not return False, they just return. I think all alternative Python
> implementations would respect this PEP.

I am Okay to the words above,  because it is a style recorded in PEP.

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




More information about the Kimchi-devel mailing list