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

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(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? >>> >> 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._... > 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.

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