doc: emitter.removeListener within emit cycle clarification #4764
Conversation
Listeners removed from an event while the event's emit cycle is underway will still be called in that cycle and the new listener array will only be used in subsequent calls. This change updates the documentation to explicitly clarify this behaviour.
thefourtheye
added
doc
|
The first line of the commit description ( Otherwise LGTM. |
| @@ -339,7 +339,8 @@ Removes the specified `listener` from the listener array for the specified | |||
| `removeListener` will remove, at most, one instance of a listener from the | |||
| listener array. If any single listener has been added multiple times to the | |||
| listener array for the specified `event`, then `removeListener` must be called | |||
| multiple times to remove each instance. | |||
| multiple times to remove each instance. Listeners removed from an event from | |||
| within the emit cycle of that event will still be called in that emit cycle. | |||
There was a problem hiding this comment.
The term emit cycle isn't defined anywhere, is it? It might be better to say that it will run all listeners attached at the time an event is emitted. This is sort of tricky to explain just in text, and a code example would be extremely helpful.
There was a problem hiding this comment.
I would argue that a unit test to prove that this happens would be good too.
|
Agreed that a unit test would be good. I'm not sure if that behavior is intentional or accidental so if we're going to document it, we'd better make sure it doesn't change :-) |
|
The original bug at nodejs/node-v0.x-archive#7872 was closed because according to the person closing the bug this is the intended behaviour. I think that this behaviour is fairly non-trivial, so if it's indeed intended, it should be documented (and if isn't, it should be fixed). The test is probably a good idea to ensure that this guarantee will be kept in the future. |
|
@drjokepu Are you working on writing the test ? I wish to help if its not already done. |
|
@vaibhav93 I'm sorry, I've been rather busy with real life work the past few weeks and it unlikely that I'll have the time to look into this the next few weeks either, so if you have time and you would like to help, feel free to do it. |
Fix based on nodejs#4764 by @drjokepu. Fixes: nodejs#4759
|
@drjokepu I've created a new branch with the documentation fix + a code example + a test for it, but I'm not sure how to proceed. Should I open a new PR? Is there a way to add a commit to your PR? |
As discussed in #4759, listeners removed from an event while the event's emit cycle is underway will still be called in that cycle and the new listener array will only be used in subsequent calls. This change updates the documentation to explicitly clarify this behaviour.