[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