From 2e98fe29010925c829072b23cb99a99860883adf Mon Sep 17 00:00:00 2001 From: Matthieu Dartiailh Date: Mon, 11 Sep 2017 08:39:33 +0200 Subject: [PATCH 1/2] src: prevent observer from propagating exceptions Exceptions are simply printed to stderr --- atom/src/member.cpp | 4 ++- atom/src/modifyguard.h | 14 -------- atom/src/observerpool.cpp | 9 ++++-- atom/src/utils.h | 1 - tests/test_observe.py | 68 +++++++++++++++++++++++++++++++++++++-- 5 files changed, 75 insertions(+), 21 deletions(-) diff --git a/atom/src/member.cpp b/atom/src/member.cpp index ce184fb5..f5881fa1 100644 --- a/atom/src/member.cpp +++ b/atom/src/member.cpp @@ -1077,7 +1077,9 @@ Member::notify( CAtom* atom, PyObject* args, PyObject* kwargs ) callable = *it; } if( !callable( argsptr, kwargsptr ) ) - return false; + { + PyErr_Print(); + } } } return true; diff --git a/atom/src/modifyguard.h b/atom/src/modifyguard.h index b1f78e07..dc6ef0ac 100644 --- a/atom/src/modifyguard.h +++ b/atom/src/modifyguard.h @@ -30,16 +30,6 @@ class ModifyGuard ~ModifyGuard() { - // If an exception occurred we store it and restore it after the - // modification have been done as otherwise it can (Python 3) cause - // boolean tests (PyObject_IsTrue) to fail for wrong reasons. - bool exception_set = false; - PyObject *type, *value, *traceback; - if( PyErr_Occurred() ){ - PyErr_Fetch(&type, &value, &traceback); - exception_set = true; - } - if( m_owner.get_modify_guard() == this ) { m_owner.set_modify_guard( 0 ); @@ -51,10 +41,6 @@ class ModifyGuard delete *it; } } - - // Restore previous exception if one was set. - if( exception_set ) - PyErr_Restore(type, value, traceback); } void add_task( ModifyTask* task ) { m_tasks.push_back( task ); } diff --git a/atom/src/observerpool.cpp b/atom/src/observerpool.cpp index f80fda02..1b6e47eb 100644 --- a/atom/src/observerpool.cpp +++ b/atom/src/observerpool.cpp @@ -7,7 +7,6 @@ |----------------------------------------------------------------------------*/ #include "observerpool.h" - namespace { @@ -220,7 +219,13 @@ ObserverPool::notify( PyObjectPtr& topic, PyObjectPtr& args, PyObjectPtr& kwargs if( obs_it->is_true() ) { if( !obs_it->operator()( args, kwargs ) ) - return false; + { + PyErr_Print(); + if( PyErr_Occurred() ) + { + PyErr_Print(); + } + } } else { diff --git a/atom/src/utils.h b/atom/src/utils.h index 264c772c..410b2866 100644 --- a/atom/src/utils.h +++ b/atom/src/utils.h @@ -19,7 +19,6 @@ namespace utils #define STR_CHECK_EXACT( obj ) PyString_CheckExact( obj ) || PyUnicode_CheckExact( obj ) #endif - inline bool basestring_check( PyObject* obj ) { diff --git a/tests/test_observe.py b/tests/test_observe.py index 223b2461..80f9d086 100644 --- a/tests/test_observe.py +++ b/tests/test_observe.py @@ -12,7 +12,7 @@ from atom.api import Atom, Int, List, Value, Event, Signal, observe -def test_static_observers(): +def test_static_observers(capsys): """Test using static observers. """ @@ -72,8 +72,9 @@ def react(self, change): del ot.ext assert not ext2.has_observer('val', ot.react) - with pytest.raises(TypeError): - ot.ext = 12 + assert not capsys.readouterr()[1] + ot.ext = 12 + assert capsys.readouterr()[1] def test_dynamic_observers(): @@ -230,5 +231,66 @@ def react(self, change): with pytest.raises(TypeError): observe(['a.b.c']) + +def test_handling_of_exceptions_in_observers(capsys): + """Test that we properly print exception occuring in observers. + + """ + class Observer(object): + + def __init__(self): + self.count = 0 + self.exc_cls = ValueError + + def react(self, change): + self.count += 1 + raise self.exc_cls() + + class NotifTest(Atom): + + val = Int() + + count = Int() + + exc_cls = Value(ValueError) + + def _observe_val(self, change): + self.count += 1 + raise self.exc_cls() + + ob = Observer() + nt = NotifTest() + nt.observe('val', ob.react) + + assert not capsys.readouterr()[1] + + # Check both static and dynamic notifiers are called even when raising + nt.val = 1 + assert ob.count == 1 + assert nt.count == 1 + + # Check we did print to stderr + assert capsys.readouterr()[1] + + class MyException(Exception): + + def __str__(self): + raise ValueError() + + ob.exc_cls = MyException + nt.exc_cls = MyException + + assert not capsys.readouterr()[1] + + # Check both static and dynamic notifiers are called even when raising + nt.val += 1 + assert ob.count == 2 + assert nt.count == 2 + + # Check we did print to stderr + err = capsys.readouterr()[1] + print(err) + assert err + # XXX add a test catching the SystemError of Python 3 # For the time being I known how to write one only using enaml From e2c7266eeea1c00a7bfc64153eea033f70e43972 Mon Sep 17 00:00:00 2001 From: Matthieu Dartiailh Date: Tue, 12 Sep 2017 18:38:19 +0200 Subject: [PATCH 2/2] atom: remove left-over previous work --- atom/src/observerpool.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/atom/src/observerpool.cpp b/atom/src/observerpool.cpp index 1b6e47eb..185055c7 100644 --- a/atom/src/observerpool.cpp +++ b/atom/src/observerpool.cpp @@ -221,10 +221,6 @@ ObserverPool::notify( PyObjectPtr& topic, PyObjectPtr& args, PyObjectPtr& kwargs if( !obs_it->operator()( args, kwargs ) ) { PyErr_Print(); - if( PyErr_Occurred() ) - { - PyErr_Print(); - } } } else