Skip to content

events: report original once() listener on removal - #66621

Open
vedchaudhari wants to merge 1 commit into
nodejs:mainfrom
vedchaudhari:events-remove-listener-once-wrapper
Open

vedchaudhari wants to merge 1 commit into
nodejs:mainfrom
vedchaudhari:events-remove-listener-once-wrapper

Conversation

@vedchaudhari

Copy link
Copy Markdown
Contributor

If a once() listener is removed while other listeners are still on the same event, the 'removeListener' event gets Node's internal once() wrapper instead of the original listener:

const EventEmitter = require('node:events');
function f() {}
function g() {}
const ee = new EventEmitter();
ee.once('x', f);
ee.on('x', g);
ee.on('removeListener', (type, listener) => console.log(listener === f));
ee.emit('x');
// before: false (listener is the internal wrapper)
// after:  true

This was fixed in #6394 (2016) and documented, but #33596 (2020) removed the unwrapping from the multi-listener path. The existing test only used a single listener, so CI didn't catch it.

This unwraps the listener the same way the single-listener path already does, and adds a test with a second listener. The stream, domain and event tests pass.

Benchmark: events/ee-add-remove.js (30 runs)
                                                                  confidence   improvement   accuracy (*)    (**)   (***)
events/ee-add-remove.js n=1000000 removeListener=0 newListener=0   **             -0.98 %   ±0.59%  ±0.78%  ±1.02%
events/ee-add-remove.js n=1000000 removeListener=1 newListener=0    *             +1.58 %   ±1.39%  ±1.86%  ±2.44%
events/ee-add-remove.js n=1000000 removeListener=0 newListener=1    *             -1.67 %   ±1.45%  ±1.94%  ±2.52%
events/ee-add-remove.js n=1000000 removeListener=1 newListener=1                  +0.08 %   ±1.42%  ±1.89%  ±2.46%

The changed line only runs when a 'removeListener' listener is attached, so the removeListener=0 rows run the same code as before and their ±1–2% is noise.

I used Claude Code to find the bug and write the fix and test. I reproduced it, reviewed the change, and ran the tests and benchmark myself.

Refs: #5551
Refs: #6394
Refs: #33596

When a `once()` listener is removed, emit the original user-defined
function in the `removeListener` event instead of the internal wrapper,
matching the behavior of the single-listener path.

Assisted-by: claude:opus-5.5
Signed-off-by: vedchaudhari <vedc2853@gmail.com>
@nodejs-github-bot nodejs-github-bot added events Issues and PRs related to EventEmitter and the events module. needs-ci PRs that need a full CI run. labels Oct 9, 2026
@codecov

codecov Bot commented Oct 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.42%. Comparing base (dd9777b) to head (7985f64).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66621      +/-   ##
==========================================
- Coverage   92.78%   90.42%   -2.36%     
==========================================
  Files         422      791     +369     
  Lines      193692   276585   +82893     
  Branches    29881    53114   +23233     
==========================================
+ Hits       179714   250110   +70396     
- Misses      13650    16872    +3222     
- Partials      328     9603    +9275     
Files with missing lines Coverage Δ
lib/events.js 99.60% <100.00%> (+6.28%) ⬆️

... and 498 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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 EventEmitter and the events module. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants