Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: removeListener event should be passed the user's listener function when using once() #5551

Closed
omsmith opened this issue Mar 3, 2016 · 2 comments
Labels
events Issues and PRs related to the events subsystem / EventEmitter.

Comments

@omsmith
Copy link
Contributor

omsmith commented Mar 3, 2016

  • Version: v4.3.2
  • Platform: Linux osmith-ubuntu-t1700 4.2.0-30-generic #36-Ubuntu SMP Fri Feb 26 00:58:07 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: events

tl;dr; I think both of these lines should pass listener.listener || listener
https://github.com/nodejs/node/blob/master/lib/events.js#L309
https://github.com/nodejs/node/blob/master/lib/events.js#L338

When using ee.once(), the user's listener function is wrapped. This is correctly accounted for during addListener when emitting the 'newListener' event, but not during removeListener and the 'removeListener' event.

I can PR later this evening - just opening the issue for the moment.

Test case:

'use strict';

const assert = require('assert');
const EventEmitter = require('events');

const ee = new EventEmitter();

function myListener () {};

let addedType = null;
let addedListener = null;
let removedType = null;
let removedListener = null;

ee.on('removeListener', (type, listener) => {
  removedType = type;
  removedListener = listener;
});
ee.on('newListener', (type, listener) => {
  addedType = type;
  addedListener = listener;
});

ee.once('foo', myListener);

ee.emit('foo');

process.on('exit', () => {
  assert.equal('foo', addedType);
  assert.equal(addedListener, myListener);
  assert.equal('foo', removedType);
  process._rawDebug('next assertion will fail');
  assert.equal(removedListener, myListener);
});
next assertion will fail

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: { [Function: g] listener: [Function: myListener] } == [Function: myListener]
    at process.<anonymous> (/home/omsmith/ee-remove-listener-test/index.js:33:9)
    at emitOne (events.js:77:13)
    at process.emit (events.js:169:7)
@mscdex mscdex added the events Issues and PRs related to the events subsystem / EventEmitter. label Mar 3, 2016
@simonkcleung
Copy link

1st PR is OK. 2nd PR has problem.
Removing the wrapper should be specified, otherwise it will remove the first listener which may not be added by "once".

omsmith added a commit to omsmith/node that referenced this issue Mar 11, 2016
When a user listens for an event using #once, their listener is wrapped
in a function which will remove it after being called. While this was
accounted for with the 'newListener' event, 'removeListener' would
receive the wrapped function.

Fixes nodejs#5551
omsmith added a commit to omsmith/node that referenced this issue Apr 11, 2016
When a user listens for an event using #once, their listener is wrapped
in a function which will remove it after being called. While this was
accounted for with the 'newListener' event, 'removeListener' would
receive the wrapped function.

Fixes nodejs#5551
@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

Fix landed in #6394

@jasnell jasnell closed this as completed Apr 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

No branches or pull requests

4 participants