Should EmitterMixin.stopListening() off all listeners (those added through on() too)?

[Reinmar]

While working on ckeditor/ckeditor5-engine#904 @scofalik noticed that stopListening() is not removing listeners added using on().

I assume it was such a case, right @scofalik?

a.on( 'x',  callback );

a.stopListening();

If this is true, then it means that you can't easily clean all listeners that you registered. My first thought was that it makes this method useless.

However, as I found that I already explained in ckeditor/ckeditor5-design#142 (sic! :D), this.on() is to be used by a class internally. So if class X needs to listen to its own events, it can use this.on(). This means that together with the class its listeners will be gone because no more events will be fired as well.

However, again, this is a theory, because we don't enforce the rule that you can fire events only on yourself. So someone may mistakenly still fire events on an already destroyed instance of class X and the callbacks will be executed. This increases a chance of a bug.

Taken that, I'd say that:

• either stopListening() should off() all listeners added by on(),
• or we should always use listenTo() and remove on() (alternatively, on() should use listenTo() internally).

I wouldn't totally remove on() because its shortness is useful. Having to always do a.listenTo( a, ...) would be cumbersome. Also, we use on() mostly in tests and half of those cases where we use it source are incorrect (listenTo() should be used with a proper subject).

So, I'm either for making on() a shortcut to a.listenTo( a ) or stopListening() offing all events. In the former case we need to figure out what off() should do. In the latter, nothing changes AFAICT.

WDYT?

[oleq]

stopListening() offing all events.

👍

I'm really surprised it works that way ATM. I'm pretty sure outsiders will be thrice as surprised because of that and I think it's a good enough reason to rectify stopListening.

[Reinmar]

Actually, please guys note that making stopListening() offing all events doesn't mean that on() can't be a shorthand for a.listenTo( a ). The latter is an additional improvement to the internals and possible simplification to the model. There are only listeners added by listenTo(), all are tracked and all are removed by stopListening().

So, this is kinda additional question – do you think that this should also be done?

[oskarwrobel]

listenTo() and once() uses on() internally.

[jodator]

I would go with anything that is more common. The stopListening() removing all events handlers looks like expected thing. We have already one "nonstandard" thing (fire vs trigger ;) )in events but we shouldn't introduce more.

[scofalik]

I'd go even further and either remove on/off pair or listenTo/stopListening pair.

Edit: Okay, maybe not that far, using .on() is sometimes useful. But let's make it a standard that we use listenTo and stopListening and leave on for specific / corner cases.

[Reinmar]

Edit: Okay, maybe not that far, using .on() is sometimes useful. But let's make it a standard that we use listenTo and stopListening and leave on for specific / corner cases.

Yes, using on() is useful in e.g. tests, with short-lived objects. The same, if an object uses it on itself. So it's more about making on() behaving nicely (being easily off-able).

[scofalik]

Okay so I see that we all agree that stopListening() should also off() events.

[Reinmar]

I R-ed @scofalik's PR: ckeditor/ckeditor5-utils#145 (comment)

The PR implements the simplest solution (forced off() on stopListening()), but for me it just made things messier. After looking at the code, I'm now more convinced that we need to clean up that module.

So, right now on() is the base method and listenTo() uses it internally. I think that it will be much clearer if listenTo() is the base method and on() is just this.listenTo( this, ... ). Similar change needs to be done with off() and stopListening().

[jodator]

OK I've dig into this and checking how @scofalik fixed memory leaks in ckeditor/ckeditor5-utils#145 PR. Also I've refactore (in t/144-new those methods so there are two: listenTo and stopListening).

In term of memory leaks we have to remember which object called listenTo.

Event callbacks will always be stored at emiter and given that it enforces that either:
a) remember to to stop listening to some object
b) call listenTo on this object
A:

this.listenTo( someEmitter, 'foo', () => {} );

// other palce in code:
this.listenTo( someEmitter, 'bar', () => {} );

// later in code: when someEmitter has reached End Of Life
this.stopListening( someEmitter );

B:

someEmitter.listenTo( someEmitter, 'foo', () => {} );
// same as:
someEmitter.on( 'foo', () => {} );

// other palce in code:
someEmitter.listenTo( someEmitter, 'bar', () => {} );

// here nothing have to be done as log no reference to `someEmitter` exists 
// it will be garbage collected.

The most problematic part of code as I see it is in listenTo mechanism that registers on which Emitters current Observer have registered callbacks so it can dereference them with stopListening(). In other words as long as Observer will not call this.stopListening( someEmitter ) that long a reference to Emitter will be in Observer thus memory will not be freed.

@pjasiun said that from documantation POV it might be clearer when there will be two separate classes: Emitter and Observer (or other names). The latter with listenTo and stopListening methods. But still we will have the same problem of where references are stored.

[Reinmar]

The B option is much better. If you're using an object which is supposed to be temporary, you should pick it as the observer (so use obj.listenTo( obj, ... )). This is safer cause you don't need to remember about this.stopListening( obj ).