EventEmitter's callback loop is mutable

RESOLVED FIXED in 1.0

Status

L20n
JS Library
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

unspecified
x86
Mac OS X

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
If one removes callaback to himself within the callback, the forEach insite EE.emit skips the next callback.

Example:

var ctx = L20n.getContext();
ctx.addResource('<browser "foo">');
ctx.addEventListener('ready', function f1() {
  console.log(1);
  ctx.removeEventListener('ready', f1);
});
ctx.addEventListener('ready', function f2() {
  console.log(2); // this never gets executed
});
ctx.addEventListener('ready', function f3() {
  console.log(3);
});
ctx.freeze();
What does node do specifically?

Going through the callbacks in reverse order should fix this, right?

Try this code:

var arr = ["a", "b", "c"];
var i = arr.length;
while(i--) {
  console.log(arr[i], i, arr);
  if (i === 1) {
    arr.splice(i, 1);
  }
}
(Assignee)

Comment 3

5 years ago
Except that we don't want to go in reverse order :)

Node clones the list and iterates over the clone:

listeners = handler.slice();
len = listeners.length;
for (i = 0; i < len; i++)
  listeners[i].apply(this, args);
Assignee: nobody → gandalf
Priority: -- → P2
Target Milestone: --- → 1.0
(Assignee)

Updated

5 years ago
Priority: P2 → P1
(Assignee)

Comment 4

5 years ago
Created attachment 758351 [details] [diff] [review]
patch
Attachment #758351 - Flags: review?(stas)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Comment on attachment 758351 [details] [diff] [review]
patch

Review of attachment 758351 [details] [diff] [review]:
-----------------------------------------------------------------

This is exactly the type of patch that I'd like to land with accompanying tests.  Let's wait with the fix until next week when we have the test runner.

::: lib/l20n/events.js
@@ +11,4 @@
>    EventEmitter.prototype.emit = function ee_emit() {
>      var args = Array.prototype.slice.call(arguments);
>      var type = args.shift();
> +    var typeListeners = this._listeners[type].slice();

If no listeners have been registered for the type, this._listeners[type] will be undefined and this line will throw.

@@ +16,4 @@
>        return false;
>      }
> +    var len = typeListeners.length;
> +    for (var i = 0; i < len; i++) {

Performance-wise, there's is almost no benefit of caching the length of the array in modern JIT engines, especially for small arrays.  Coding style-wise, I don't think we cache the length elsewhere, so I'd probably change that to 

  for (var i = 0; i < typeListeners.length; i++)
Attachment #758351 - Flags: review?(stas) → review-
(Assignee)

Comment 6

5 years ago
(In reply to Staś Małolepszy :stas from comment #5)
> ::: lib/l20n/events.js
> @@ +11,4 @@
> >    EventEmitter.prototype.emit = function ee_emit() {
> >      var args = Array.prototype.slice.call(arguments);
> >      var type = args.shift();
> > +    var typeListeners = this._listeners[type].slice();
> 
> If no listeners have been registered for the type, this._listeners[type]
> will be undefined and this line will throw.

Good catch! 


> @@ +16,4 @@
> >        return false;
> >      }
> > +    var len = typeListeners.length;
> > +    for (var i = 0; i < len; i++) {
> 
> Performance-wise, there's is almost no benefit of caching the length of the
> array in modern JIT engines, especially for small arrays.  Coding
> style-wise, I don't think we cache the length elsewhere, so I'd probably
> change that to 
> 
>   for (var i = 0; i < typeListeners.length; i++)

sure
(Assignee)

Comment 7

5 years ago
Created attachment 758617 [details] [diff] [review]
patch v2
Attachment #758351 - Attachment is obsolete: true
Attachment #758617 - Flags: review?(stas)
Attachment #758617 - Flags: review?(stas) → review+
(Assignee)

Comment 8

5 years ago
https://github.com/l20n/l20n.js/commit/05c19ba1479fe0e2b7b6ac37c100a38bcbe0d68c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.