Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add notifier for wrapping user handler for the observer framework #1000

Merged
merged 63 commits into from
May 4, 2020
Merged
Show file tree
Hide file tree
Changes from 61 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
db6428e
Craete TraitEventNotifier and make it do basic things
kitchoi Apr 7, 2020
fa4e141
Unexpected exceptions are captured and logged
kitchoi Apr 7, 2020
febe308
Implement equals
kitchoi Apr 7, 2020
94c62b3
Implement add_to
kitchoi Apr 7, 2020
117902e
Disable notifier if the target is deleted.
kitchoi Apr 7, 2020
600602d
Rename IObservableObject -> IObservable
kitchoi Apr 8, 2020
52ccbca
Fix docstring as target cannot actually be anything
kitchoi Apr 8, 2020
f7ae36f
Handlers are compared with identity
kitchoi Apr 8, 2020
5fce1ea
Reorganize test cases
kitchoi Apr 8, 2020
4c6c041
Implement remove_from
kitchoi Apr 8, 2020
35bd119
remove_from will decrement reference count.
kitchoi Apr 8, 2020
af89c24
Add test to justify the use of weakref
kitchoi Apr 8, 2020
ca66b7e
Add test for adding two different notifiers
kitchoi Apr 8, 2020
e5d1756
Add test to highlight the difference between no target tracked from t…
kitchoi Apr 8, 2020
53ef3d3
Add test for when remove_from raises
kitchoi Apr 8, 2020
be5b33e
Add test for differential removal of notifiers
kitchoi Apr 8, 2020
5c88767
Raise for race-condition, unable to test though
kitchoi Apr 8, 2020
cf8507e
Use weakref for method
kitchoi Apr 8, 2020
2590c71
Silence notifier if the weakref-ed method is deleted
kitchoi Apr 8, 2020
9b7e007
Introduce push_exception_handler and pop_exception_handler for observer
kitchoi Apr 8, 2020
81afc1a
Fix test for when the handler is deleted while notifier is called.
kitchoi Apr 8, 2020
6de85bd
Add docstring for __call__
kitchoi Apr 8, 2020
bd70167
Reformat docstring
kitchoi Apr 8, 2020
d41db28
We always have a target, remove the None branch
kitchoi Apr 8, 2020
45cdf11
Add tests for the exception handlers
kitchoi Apr 8, 2020
266f42a
Improve docstring
kitchoi Apr 8, 2020
6651da6
Make module name private
kitchoi Apr 8, 2020
fdc4d65
Add test to ensure dispatcher is used.
kitchoi Apr 8, 2020
2485c5b
Implement the logic of prevent_event
kitchoi Apr 8, 2020
167da0e
Refactor tests
kitchoi Apr 8, 2020
a9f01db
Rename basic_dispatch -> dispatch_here
kitchoi Apr 8, 2020
c06ef76
Add docstring for not_prevent_event
kitchoi Apr 8, 2020
18552b0
Add docstring for DummyObservable
kitchoi Apr 8, 2020
84759dc
Call weakref once instead of twice
kitchoi Apr 8, 2020
61b377c
Extend docstring on weakref.WeakMethod
kitchoi Apr 8, 2020
c405c4f
More docstring improvements
kitchoi Apr 8, 2020
fde6684
Move check so it reads more naturally
kitchoi Apr 8, 2020
4a59e04
Make a shallow copy before modification
kitchoi Apr 8, 2020
08ca08a
Merge branch 'master' into traits-observe-notifier
kitchoi Apr 9, 2020
03b8e7c
Target can be any object
kitchoi Apr 9, 2020
563ea1a
Catch unexpected use case
kitchoi Apr 9, 2020
13b4afc
Remove unused logger
kitchoi Apr 9, 2020
fda97b7
Update docstring
kitchoi Apr 9, 2020
fe52eb0
Extend test logic
kitchoi Apr 9, 2020
2f73866
Expand description on reference counting.
kitchoi Apr 9, 2020
513f278
More docstring clean up
kitchoi Apr 9, 2020
eebddb3
Nit: avoid using the word target again
kitchoi Apr 9, 2020
8bf7696
ValueError -> RuntimeError
kitchoi Apr 9, 2020
0e57e6a
Add Raises sections to docstring
kitchoi Apr 9, 2020
787c6ce
Fix test messed up during refactoring
kitchoi Apr 9, 2020
a0b9ad9
Fix instance methods comparison
kitchoi Apr 20, 2020
74cacff
Use a custom exception so exceptions can be made later
kitchoi Apr 20, 2020
3c8ef5e
Reinstate the default last resort: log to stderr
kitchoi Apr 20, 2020
f4e225d
Sanity check on handler early
kitchoi Apr 20, 2020
c17a849
Move parameters to class docstring
kitchoi Apr 20, 2020
961e0a3
Extend Raises section
kitchoi Apr 20, 2020
5eb6053
Rename _iobservable -> _i_observable to be consistent with another PR
kitchoi Apr 20, 2020
13d92a1
Nit: Define Parameters in the class docstring
kitchoi Apr 20, 2020
40ec482
Merge branch 'master' into traits-observe-notifier
kitchoi Apr 22, 2020
9614866
Improve docstring
kitchoi Apr 29, 2020
d65e756
Newline between imports and copyright
kitchoi Apr 29, 2020
fe7e4ab
Compare dispatcher in equals
kitchoi May 4, 2020
367ca3d
Define event_factory in-place in two of the tests
kitchoi May 4, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions traits/observers/_exception_handling.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
# (C) Copyright 2005-2020 Enthought, Inc., Austin, TX
# All rights reserved.
#
# This software is provided without warranty under the terms of the BSD
# license included in LICENSE.txt and may be redistributed only under
# the conditions described in the aforementioned license. The license
# is also available online at http://www.enthought.com/licenses/BSD.txt
#
# Thanks for using Enthought open source!

# This module provides the push_exception_handler and pop_exception_handler
# for the observers.

import logging
import sys


class ObserverExceptionHandler:
""" State for an exception handler.

Parameters
----------
handler : callable(event) or None
A callable to handle an event, in the context of
an exception. If None, the exceptions will be logged.
reraise_exceptions : boolean
Whether to reraise the exception.
"""

def __init__(self, handler, reraise_exceptions):
self.handler = handler if handler is not None else self._log_exception
self.reraise_exceptions = reraise_exceptions
self.logger = None

def _log_exception(self, event):
""" A handler that logs the exception with the given event.

Parameters
----------
event : object
An event object emitted by the notification.
"""
if self.logger is None:
self.logger = logging.getLogger("traits")
handler = logging.StreamHandler()
handler.setFormatter(logging.Formatter("%(message)s"))
self.logger.addHandler(handler)
self.logger.setLevel(logging.ERROR)

self.logger.exception(
"Exception occurred in traits notification handler "
"for event object: %r",
event,
)


class ObserverExceptionHandlerStack:
""" A stack of exception handlers.

Parameters
----------
handlers : list of ObserverExceptionHandler
The last item is the current handler.
"""

def __init__(self):
self.handlers = []

def push_exception_handler(
self, handler=None, reraise_exceptions=False):
""" Push a new exception handler into the stack. Making it the
current exception handler.

Parameters
----------
handler : callable(event) or None
A callable to handle an event, in the context of
an exception. If None, the exceptions will be logged.
reraise_exceptions : boolean
Whether to reraise the exception.
"""
self.handlers.append(
ObserverExceptionHandler(
handler=handler, reraise_exceptions=reraise_exceptions,
)
)

def pop_exception_handler(self):
""" Pop the current exception handler from the stack.

Raises
------
IndexError
If there are no handlers to pop.
"""
return self.handlers.pop()

def handle_exception(self, event):
""" Handles a traits notification exception using the handler last pushed.

Parameters
----------
event : object
An event object emitted by the notification.
"""
_, excp, _ = sys.exc_info()
try:
handler_state = self.handlers[-1]
except IndexError:
handler_state = ObserverExceptionHandler(
handler=None,
reraise_exceptions=False,
)

handler_state.handler(event)
if handler_state.reraise_exceptions:
raise excp


_exception_handler_stack = ObserverExceptionHandlerStack()
push_exception_handler = _exception_handler_stack.push_exception_handler
pop_exception_handler = _exception_handler_stack.pop_exception_handler
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking ahead... we will need to expose this publicly, but they will need to be wrapped under some compatible layer to reconcile the difference between these functions and the ones in traits.trait_notifiers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A compatibility layer may not be needed if the API is good here: pushing and popping exception handlers is rare (mainly used in the test-support code, I think).

handle_exception = _exception_handler_stack.handle_exception
14 changes: 14 additions & 0 deletions traits/observers/_exceptions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# (C) Copyright 2005-2020 Enthought, Inc., Austin, TX
# All rights reserved.
#
# This software is provided without warranty under the terms of the BSD
# license included in LICENSE.txt and may be redistributed only under
# the conditions described in the aforementioned license. The license
# is also available online at http://www.enthought.com/licenses/BSD.txt
#
# Thanks for using Enthought open source!


class NotifierNotFound(Exception):
""" Raised when a notifier cannot be found."""
pass
29 changes: 29 additions & 0 deletions traits/observers/_i_observable.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# (C) Copyright 2005-2020 Enthought, Inc., Austin, TX
# All rights reserved.
#
# This software is provided without warranty under the terms of the BSD
# license included in LICENSE.txt and may be redistributed only under
# the conditions described in the aforementioned license. The license
# is also available online at http://www.enthought.com/licenses/BSD.txt
#
# Thanks for using Enthought open source!


class IObservable:
""" Interface for objects that can emit notifications for the observer
system.
"""

def _notifiers(self, force_create):
kitchoi marked this conversation as resolved.
Show resolved Hide resolved
""" Return a list of callables where each callable is a notifier.
The list is expected to be mutated for contributing or removing
notifiers from the object.

Parameters
----------
force_create: boolean
It is added for compatibility with CTrait.
It should not be used otherwise.
"""
raise NotImplementedError(
"Observable object must implement _notifiers")
218 changes: 218 additions & 0 deletions traits/observers/_trait_event_notifier.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,218 @@
# (C) Copyright 2005-2020 Enthought, Inc., Austin, TX
# All rights reserved.
#
# This software is provided without warranty under the terms of the BSD
# license included in LICENSE.txt and may be redistributed only under
# the conditions described in the aforementioned license. The license
# is also available online at http://www.enthought.com/licenses/BSD.txt
#
# Thanks for using Enthought open source!

from functools import partial
import types
import weakref

from traits.observers._exception_handling import handle_exception
from traits.observers._exceptions import NotifierNotFound


class TraitEventNotifier:
""" Wrapper for invoking user's handler for a trait change event.

An instance of ``TraitEventNotifier`` is a callable to be contributed
to an instance of ``IObserverable``, e.g. ``CTrait``, ``TraitList`` etc.,
such that it will be called when an observerable emits notificaitons for
changes. The call signature is defined by the observable object
and may vary. It is the responsibility of the ``event_factory`` to adapt
the varying call signatures and create an event object to be given
to the user's handler.

A ``TraitEventNotifier`` keeps a reference count in order to address
situations where a same object is repeated inside a container
and one would not want to fire the same change handler multiple times
(see enthought/traits#237). For that purpose, a notifier keeps track of
the ``HasTraits`` instance (called ``target``) on which the user applies
the observers, keeps a reference count internally, and it also needs to
determine whether another notifier refers to the same change handler and
``HasTraits`` instance.

Since there is only one reference count associated with a notifier,
each notifier is expected to be added to only one observable.

Parameters
----------
handler : callable(event)
The user's handler to receive the change event.
The event object is returned by the ``event_factory``.
If the handler is an instance method, then a weak reference is
created for the method. If the instance is garbage collected,
the notifier will be muted.
target : object
An object for defining the context of the notifier.
A weak reference is created for the target.
If the target is garbage collected, the notifier will be muted.
This target is typically an instance of ``HasTraits`` and will be
seen by the user as the "owner" of the change handler.
This is also used for distinguishing one notifier from another
notifier wrapping the same handler.
event_factory : callable(*args, **kwargs) -> object
A factory function for creating the event object to be sent to
the handler. The call signature must be compatible with the
call signature expected by the observable this notifier is used
with. e.g. for CTrait, the call signature will be
``(object, name, old, new)``.
prevent_event : callable(event) -> boolean
A callable for controlling whether the user handler should be
invoked. It receives the event created by the event factory and
returns true if the event should be prevented, false if the event
should be fired.
dispatcher : callable(handler, event)
A callable for dispatching the handler, e.g. on a different
thread or on a GUI event loop. ``event`` is the object
created by the event factory.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following comment is still valid but GitHub marked it outdated because I moved the docstring after making the comment: #1000 (comment) :

I am a bit tempted to group handler, target, dispatcher into another object. They do always go together in other places. That object can provide the methods for equals and for whether or not the weakrefs are still live.
We could also refactor this later when all the other pieces are here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this sentiment, but also that this is sufficiently deep internals that we can change it later without major issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right this is not going to affect the API users will interact with, so I'd be happy to do the refactoring in a separate PR, I will open an issue for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #1063


Raises
------
ValueError
If the handler given is not a callable.
"""

def __init__(
self, *, handler, target,
event_factory, prevent_event, dispatcher):

if not callable(handler):
raise ValueError(
"handler must be a callable, got {!r}".format(handler))

# This is such that the notifier does not prevent
# the target from being garbage collected.
self.target = weakref.ref(target)

if isinstance(handler, types.MethodType):
self.handler = weakref.WeakMethod(handler)
Copy link
Contributor Author

@kitchoi kitchoi Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously (in the PoC) there was a callback in both weakref for removing this notifier from the observable when either the handler or the target is deleted. That requires keeping a reference to the list of notifiers in the observable. If we don't do the cleanup, we may have some silenced notifiers lying around if the code that puts up the notifiers don't do a good job of taking them down (e.g. traitsui). This may be a bit inefficient, but I am not sure how common this scenario is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

19 days after making this comment: I am now more in favour of not having a weakref callback for removing the notifier. Without the weakref callback, there is only one way to remove the notifier: remove_from. Currently remove_from raises an exception if the notifier to be removed is not found. This is very helpful for catching programming error. If the code is right, every call to add_to should be undone by a call to remove_from, like open bracket and close bracket. If we had a weakref callback to remove the notifier from the observable, this introduces a second way for notifiers to be removed, from a random thread, and remove_from can no longer raise an exception if the notifier cannot be found.

else:
self.handler = partial(_return, value=handler)
self.dispatcher = dispatcher
self.event_factory = event_factory
self.prevent_event = prevent_event
# Reference count to avoid adding multiple equivalent notifiers
# to the same observable.
self._ref_count = 0

def __call__(self, *args, **kwargs):
""" Called by observables. The call signature will vary and will be
handled by the event factory.
"""
if self.target() is None:
# target is deleted. The notifier is disabled.
return

# Hold onto the reference while invoking the handler
handler = self.handler()

if handler is None:
# The instance method is deleted. The notifier is disabled.
return

event = self.event_factory(*args, **kwargs)
if self.prevent_event(event):
return
try:
self.dispatcher(handler, event=event)
except Exception:
handle_exception(event)

def add_to(self, observable):
""" Add this notifier to an observable object.

If an equivalent notifier exists, the existing notifier's reference
count is bumped. Hence this method is not idempotent.
N number of calls to this ``add_to`` must be matched by N calls to the
``remove_from`` method in order to completely remove a notifier from
an observable.

Parameters
----------
observable : IObservable
An object for adding this notifier to.

Raises
------
RuntimeError
If the internal reference count is not zero and an equivalent
notifier is not found in the observable.
"""
notifiers = observable._notifiers(True)
for other in notifiers:
if self.equals(other):
other._ref_count += 1
break
else:
# It is not a current use case to share a notifier with multiple
# observables. Using a single reference count will tie the lifetime
# of the notifier to multiple objects.
if self._ref_count != 0:
raise RuntimeError(
"Sharing notifiers across observables is unexpected."
)
notifiers.append(self)
self._ref_count += 1

def remove_from(self, observable):
""" Remove this notifier from an observable object.

If an equivalent notifier exists, the existing notifier's reference
count is decremented and the notifier is only removed if
the count is reduced to zero.

Parameters
----------
observable : IObservable
An object for removing this notifier from.

Raises
------
RuntimeError
If the reference count becomes negative unexpectedly.
NotifierNotFound
If the notifier is not found.
"""
notifiers = observable._notifiers(True)
for other in notifiers[:]:
if self.equals(other):
if other._ref_count == 1:
notifiers.remove(other)
other._ref_count -= 1
if other._ref_count < 0:
raise RuntimeError(
"Reference count is negative. "
"Race condition?"
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR, I was able to run into some kind of race conditions sometimes with the following test, but this particular exception has not yet occurred.


    def test_race_condition(self):
        dummy = DummyObservable()
        dummy.notifiers = [str] * 100
        notifier = create_notifier()

        def func1(num):
            for _ in range(num // 4):
                notifier.add_to(dummy)
            for _ in range(num // 4):
                notifier.remove_from(dummy)
            for _ in range(num // 4):
                notifier.add_to(dummy)
            for _ in range(num // 4):
                notifier.remove_from(dummy)

        import concurrent.futures
        with concurrent.futures.ThreadPoolExecutor(max_workers=60) as executor:
            list(executor.map(func1, range(10, 100)))

        self.assertEqual(len(dummy.notifiers), 100)
        self.assertEqual(notifier._ref_count, 0)

The last two assertions fail sometimes, the "Notifier not found." exception appears sometimes, and the "Sharing notifiers across observables is unexpected." error in add_to appears sometimes too.

break
else:
raise NotifierNotFound("Notifier not found.")

def equals(self, other):
""" Return true if the other notifier is equivalent to this one.

Parameters
----------
other : any

Returns
-------
boolean
"""
if other is self:
return True
if type(other) is not type(self):
return False
return (
self.handler() == other.handler()
and self.target() is other.target()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another discussion point: I think currently if one calls on_trait_change with the same arguments except for dispatch, the first one wins:

foo.on_trait_change(handler, "name", dispatch="same")
foo.on_trait_change(handler, "name", dispatch="ui")

The second call is ignored because "dispatch" is not considered for differentiating notifiers. Here we could differentiate them, and I am tempted to think that we should. In other words, calling the following will result in two notifiers, one to be dispatched on the same thread, and the second one to be dispatched on the GUI event loop:

obj.observe(handler, "name", dispatch="same")
obj.observe(handler, "name", dispatch="ui")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favour of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I will add this one line for comparing the dispatch callable (and its test).

)


def _return(value):
return value
Loading