TraitList, TraitDict, TraitSet don't catch notification errors

[corranwebster]

Currently all three of the new container classes dispatch notification via a simple:

for notifier in self.notifiers:
    notifier(self, index, removed, added)

This means that if a single notifier fails, all subsequent notifiers will not be called. This is different from the behaviour of standard Trait notification.

Trait container notifiers should have similar behaviour to trait notifiers, and if we do not fix this may lead to subtle bugs in the new observer framework where an unrelated error in a notification may mean that observers are not hooked and unhooked correctly.

[mdickinson]

I agree that we should be catching and logging errors, but it's not clear to me whether the right place to do that is here or in the notifiers themselves. It may be here.

[mdickinson]

Question: does this represent a regression since Traits 6.0.0? What part of the 6.0 machinery handles exceptions?

[kitchoi]

I think looping over the notifiers and exit upon the first one fails is consistent with call_notifiers in ctraits.c: https://github.com/enthought/traits/blob/92804498e306dd11091b8e4607cccc3352358130/traits/ctraits.c#L2256-L2259

Existing on_trait_change machinery is capturing error in the notifiers, for example: https://github.com/enthought/traits/blob/92804498e306dd11091b8e4607cccc3352358130/traits/trait_notifiers.py#L349-L354

Observers are catching the error in the notifier: https://github.com/enthought/traits/pull/1000

I actually prefer not capturing the error in TraitList and friends. It is useful to be able to selectively silence the exception where it is appropriate. The observer logic for hooking and unhooking notifiers rely on exceptions to make the entire operation atomic: https://github.com/enthought/traits/pull/1023 See tests test_add_notifier_atomic and test_remove_atomic.

[kitchoi]

The observer logic for hooking and unhooking notifiers rely on exceptions to make the entire operation atomic: #1023 See tests test_add_notifier_atomic and test_remove_atomic.

Actually, the exceptions there are independent of whether TraitList.notify raises or not. I am sorry for the confusion. Please ignore this particular point.

[kitchoi]

An example might be useful here, consider this:

class Driver(HasTraits):
    name = Str()

class Car(HasTraits):
    owners = List()

class TestIncompatibleExpressin(unittest.TestCase):

    def test_incompatible_expression(self):
        car = Car()

        name_changed = mock.Mock()
        age_changed = mock.Mock

        # there are no age trait on driver!
        car.observe(age_changed, "owners:items:age")
        car.observe(name_changed, "owners:items:name")

        driver = Driver(name="Dave")

        # should this raise because "age" is not defined on Driver?
        car.owners.append(driver)

        driver.name = "Danny"
        self.assertEqual(name_changed.call_count, 1)

Option 1: car.owners.append(driver) won't raise, and the change handler on Driver.name would still work. Option 2: car.owners.append(driver) will raise because the observer "owners:items:age" will complain the "age" trait is not found. The rest of the code is not run.

Even if we want to go with Option 1, the exception can be captured in notifiers. It still does not need to happen in TraitList.notify. I'd prefer option 2 because it is useful for capturing human error, e.g. typo.

[kitchoi]

Option 3: Let the user choose :)

If I change "owners:items:age" to trait("owners", notify=False).list_items(notify=False).trait("age", optional=True) (see https://github.com/enthought/traits/pull/969), then the above test would pass. The notifier does not capture the error, TraitList does not either. The exception capturing happens deeper in the stack where the instance trait is requested. (sorry I have just been talking to myself there...)

[mdickinson]

Bumping to 6.2.0 after discussion; we need to get this release wrapped up, and this can still be changed for 6.2.0 without too much disruption.

[mdickinson]

Bumping.