Behaviour of ObservableMixin when its `off` method is called while its `fire` method is currently iterating over listeners

[serenity-77]

Suppose we have two listeners on "close" event.

import txaio
from autobahn.util import ObservableMixin

txaio.use_twisted()

class Foo(ObservableMixin):

    def __init__(self):
        self.set_valid_events(["someEvent"])

    def doSomething(self):
        self.fire("someEvent", self)


def listener1(foo):
    f.off("someEvent", listener1)
    print("listener1 called")

def listener2(foo):
    print("listener2 called")

f = Foo()

f.on("someEvent", listener1)
f.on("someEvent", listener2)

f.doSomething()

This will output:

listener1 called

This means, when ObservableMixin off method is called inside listener1, except for last listeners, then the next listeners (listener2) will never get called.

I think this is because ObservableMixin off method is called while ObservableMixin fire method is iterating over current listeners of event..

[oberstet]

@meejah any hints on ^?

[meejah]

fire() does indeed just "blindly" iterate over its listeners .. so it's not re-entrant (in the sense used above). An easy fix would be to iterate over a copy of the list .. or detect that situation in off() and do a callLater(0, ...) there if it's true (which would only affect the probably-rare case of removing a listener in a notification method).

[serenity-77]

fire() does indeed just "blindly" iterate over its listeners .. so it's not re-entrant (in the sense used above). An easy fix would be to iterate over a copy of the list .. or detect that situation in off() and do a callLater(0, ...) there if it's true (which would only affect the probably-rare case of removing a listener in a notification method).

def listener1(foo):
    reactor.callLater(0, lambda: f.off("someEvent", listener1))
    print("listener1 called")

Using callLater indeed can solve it.

But I ended up with this quick fix, by extending ObservableMixin, which first copy all listeners and iterate over them.

import txaio
from autobahn.util import ObservableMixin as _ObservableMixin

class ObservableMixin(_ObservableMixin):

    def fire(self, event, *args, **kwargs):
        if self._listeners is None:
            return txaio.create_future(result=[])

        self._check_event(event)

        handlers = []
        for handler in self._listeners.get(event, []):
            handlers.append(handler)

        res = []

        for handler in handlers:
            future = txaio.as_future(handler, *args, **kwargs)
            res.append(future)
        if self._parent is not None:
            res.append(self._parent.fire(event, *args, **kwargs))
        d_res = txaio.gather(res, consume_exceptions=False)
        self._results[event] = d_res
        return d_res

And it seems works as i expected

[oberstet]

I'll keep the issue open as a "docs issue": we could at least add @meejah notes to the doc string of ObservableMixin

which first copy all listeners and iterate over them.

just a note: this might result in a performance bottleneck on high load scenarios. the ObservableMixin event handling is a high-frequency code path deep inside the library

[meejah]

Yeah, my suggestion to use callLater was thinking that the "normal" path is not to mess with on() / off() in the actual notification methods and thus make the "odd" case the one that "pays a price"; copying the list on every notification could indeed be a performance issue (as @oberstet notes, calling fire() is a high-frequency code-path at the core of many operations).

Ideally, we'd have some idea of the actual performance impact of this .. at the very least, the docs should be updated (i.e. "don't call off() from a handler").