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

console: don't attach unnecessary error handlers #27691

Merged
merged 1 commit into from
May 16, 2019
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented May 14, 2019

A noop error handler is attached to the console's stream on write. The handler is then immediately removed after the write. This commit skips adding the error handler if one already exists.

Fixes: #27687

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label May 14, 2019
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a regression test?

@cjihrig
Copy link
Contributor Author

cjihrig commented May 14, 2019

Can you add a regression test?

Sure. Done!

@mscdex
Copy link
Contributor

mscdex commented May 14, 2019

I think the test can be simplified further to something like:

'use strict';
const common = require('../common');
const { Worker, isMainThread } = require('worker_threads');
const EventEmitter = require('events');

if (isMainThread) {
  process.on('warning', common.mustNotCall('unexpected warning'));
  for (let i = 0; i < EventEmitter.defaultMaxListeners; ++i) {
    (new Worker(__filename)).on('exit', () => {
      console.log('a');
    });
  }
}

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 14, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 14, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/23118/

EDIT(cjihrig): CI was yellow.

A noop error handler is attached to the console's stream on
write. The handler is then immediately removed after the write.
This commit skips adding the error handler if one already
exists.

PR-URL: nodejs#27691
Fixes: nodejs#27687
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@cjihrig cjihrig merged commit cca375f into nodejs:master May 16, 2019
@cjihrig cjihrig deleted the 27687 branch May 16, 2019 15:58
targos pushed a commit that referenced this pull request May 17, 2019
A noop error handler is attached to the console's stream on
write. The handler is then immediately removed after the write.
This commit skips adding the error handler if one already
exists.

PR-URL: #27691
Fixes: #27687
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. console Issues and PRs related to the console subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

node v12.2.0 worker_threads worker number > 10 EventEmitter warning
9 participants