emitter.listeners('ev') should return a copy, not the actual array

[isaacs]

It makes a lot of headaches in many edge cases to expose a reference to the listeners array.

It would be better if emitter.listeners('ev') returned a copy of the array, not the actual thing. Then we could change it, without worrying about consistency guarantees or breaking userland code.

[piscisaureus]

+1 +1 +1 +1

[isaacs]

Foretold in the docs on e72addc.

[adammw]

032fc42 changed readline.js to be compatible with this change, however doesn't copying the array introduce an extra overhead for when only the length of the array is required?

Perhaps a separate listenerCount function or property could be introduced that directly gets the length of the underlying array without having to copy the array first for these sort of uses.

[dynajoe]

I think the listenerCount idea might be a bit misleading. Considering that listenerCount may not be consistent with the length of a copied array. The overhead of making a shallow copy of the array seems minimal to me vs the potential for inconsistent data.

[adammw]

@joeandaverde: Slightly confused... how would there be inconsistent data? Your probably correct as I don't know too much about Node internals, but shouldn't array.length === array.slice(0).length?

[dynajoe]

You are correct indeed.

Consider this:

listeners = emitter.listeners('foo')
emitter.removeAllListeners('foo')

if you're iterating over the listeners array with the count retrieved from emitter.listenerCount then the size MAY not correspond with the size of listeners.length.

There's just more source for confusion when accessing the size of the array separately from the actual array.

[adammw]

My .listenerCount proposal was not supposed to be used if you already had listeners from the emitter.listeners(): there's no point then and you are correct that listeners.lengthshould be used when iterating over the array and definitely not .listenerCount.
However in the case where a library wants to remove itself or do some other wok when no one is listening to it, such as the readline module, the entire array is not actually needed, just the count of listeners to know if there are any listeners. As such, in these situations the overhead of a copy just to get a length seems like it could be avoided with a property/function to directly access the length.

[bnoordhuis]

Fixed in 20e12e4.

[mscdex]

I agree with adammw. There are times when having this overhead is unnecessary.

Another example would be if you just want to check if a particular listener currently exists for some event. You shouldn't be forced to obtain a whole new copy of the listeners array just to determine this.

If this commit is here to stay, would pull requests be accepted to incorporate into EventEmitter some additional functions that operate directly on the original array to reduce overhead (e.g. emitter.hasListener('event', listener))?

Or heck, how about a function that does return the original reference to the array (e.g. emitter.listenersRef('event') or emitter.listeners('event', true)) instead?

[bnoordhuis]

If this commit is here to stay, would pull requests be accepted to incorporate into EventEmitter some additional functions that operate directly on the original array to reduce overhead (e.g. emitter.hasListener('event', listener))?

No. It reeks of premature optimization. Conclusively demonstrate a big performance drop-off in a real-world scenario first.

[isaacs]

@bnoordhuis ++. Show that this is a real problem, and we'll decide how to address it. Until then, we're going to decide not to address it.

[adammw]

Makes sense not to fix anything that isn't broken, I was just trying to pitch in any possible optimisations I could see. I've tried to benchmark if there is any difference (https://gist.github.com/3759971), and the result was about ~50ms difference over 1000000 events, but I'm not sure if they are valid because perhaps my test isn't actually written to test it correctly. For reference the emitKeypressEvents being used in the test is straight from readline.js.

I personally prefer my original idea of having a removedListener event (#3243) as I predict that it would have even less overhead as it doesn't need to check for listeners upon every emitted data, it just gets told when it needs to check.

[bnoordhuis]

I added a 'removeListener' event in #3806 but that hasn't landed yet. I guess I should apply the final spit and polish and merge it.

[adammw]

Now removeListener is in master, could someone who is good at benchmarking see if adammw/node@6b03921 has any positive or negative performance effect?

[bnoordhuis]

@adammw I seriously doubt that it makes any difference one way or the other. That functions gets called maybe ten times a second (if you're a really fast typist) and you never have more than one or two listeners.