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: avoid adding infinite error listeners #16770

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Nov 5, 2017

If the console destination is a unix pipe (net.Socket), write() is
async. If the destination is broken, we are adding an 'error' event
listener to avoid a process crash. This PR makes sure that we are adding
that listener only once.

Fixes: #16767

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

console

@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Nov 5, 2017
@mcollina
Copy link
Member Author

mcollina commented Nov 5, 2017

@gibfahn this needs to go into node 8.

process.on('warning', common.mustNotCall);

stream.cork();
for (let i = 0; i < 20; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the loop limit can be something like EventEmitter.defaultMaxListeners + 1 instead of a hardcoded value?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

If the console destination is a unix pipe (net.Socket), write() is
async. If the destination is broken, we are adding an 'error' event
listener to avoid a process crash. This PR makes sure that we are adding
that listener only once.

Fixes: nodejs#16767
@mcollina
Copy link
Member Author

mcollina commented Nov 5, 2017

@gibfahn
Copy link
Member

gibfahn commented Nov 5, 2017

@gibfahn this needs to go into node 8.

So this seems serious enough that it should go into the next Node 8.x release (planned for Tuesday). Thoughts @nodejs/lts?

If so it'd be great to get this landed ASAP.

@addaleax
Copy link
Member

addaleax commented Nov 5, 2017

@gibfahn I think it’s okay to include this in an LTS without letting it sit in a Current release, but tbh that’s more because of a low chance of breaking anything rather than any other reason.

@gibfahn
Copy link
Member

gibfahn commented Nov 5, 2017

@addaleax do you think it's worth rushing the landing of this to get it into the next release?

@addaleax
Copy link
Member

addaleax commented Nov 5, 2017

@gibfahn Can’t tell whether it’s worth it, but I think it’s the kind of bugfix that shouldn’t need to wait 4 weeks to be fixed due to process…

@mcollina
Copy link
Member Author

mcollina commented Nov 5, 2017

I think it should go in 8 asap. As @addaleax said, this is a low risk change. When is the next release planned?

@gibfahn
Copy link
Member

gibfahn commented Nov 5, 2017

@mcollina Tuesday, see #16783

@mcollina
Copy link
Member Author

mcollina commented Nov 6, 2017

And after that?
If we are ok with it we can land it tomorrow.

@mcollina
Copy link
Member Author

mcollina commented Nov 6, 2017

Are we ok to fast-track this? cc @nodejs/tsc

@gibfahn
Copy link
Member

gibfahn commented Nov 6, 2017

And after that?

Probably another couple of weeks.

@sam-github
Copy link
Contributor

Can this code get confused if the end-user adds an error handler, and then later removes it? That is, the error handler count is greater than 0, but only briefly?

@jasnell
Copy link
Member

jasnell commented Nov 6, 2017

Definitely good with getting this landed quickly. It's likely safe to let it stand for a week or two before landing in LTS.

@mcollina
Copy link
Member Author

mcollina commented Nov 6, 2017

Can this code get confused if the end-user adds an error handler, and then later removes it? That is, the error handler count is greater than 0, but only briefly?

no. Emitting 'error' is done only once, and it is just deferred to the next tick: https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L398.

This condition happens only when _writev is involved, as multiple writes are going to error at the same time.

@@ -83,7 +83,10 @@ function createWriteErrorHandler(stream) {
// an `error` event. Adding a `once` listener will keep that error
// from becoming an uncaught exception, but since the handler is
// removed after the event, non-console.* writes won’t be affected.
stream.once('error', noop);
// we are only adding noop if there is no one else listening for 'error'
Copy link
Contributor

Choose a reason for hiding this comment

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

if this can happen only with writev(), its worth mentioning specifically in the comment

@mcollina
Copy link
Member Author

mcollina commented Nov 8, 2017

Landed as d82bedc.

@mcollina mcollina closed this Nov 8, 2017
@mcollina mcollina deleted the fix-16767 branch November 8, 2017 11:45
mcollina added a commit that referenced this pull request Nov 8, 2017
If the console destination is a unix pipe (net.Socket), write() is
async. If the destination is broken, we are adding an 'error' event
listener to avoid a process crash. This PR makes sure that we are adding
that listener only once.

Fixes: #16767

PR-URL: #16770
Fixes: #16767
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
gibfahn pushed a commit that referenced this pull request Nov 8, 2017
If the console destination is a unix pipe (net.Socket), write() is
async. If the destination is broken, we are adding an 'error' event
listener to avoid a process crash. This PR makes sure that we are adding
that listener only once.

Fixes: #16767

PR-URL: #16770
Fixes: #16767
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
If the console destination is a unix pipe (net.Socket), write() is
async. If the destination is broken, we are adding an 'error' event
listener to avoid a process crash. This PR makes sure that we are adding
that listener only once.

Fixes: #16767

PR-URL: #16770
Fixes: #16767
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
gibfahn added a commit that referenced this pull request Dec 5, 2017
Notable Changes:

- **console**:
  - avoid adding infinite error listeners (Matteo Collina) [#16770](https://github.com/nodejs/n
de/pull/16770)
- **http2**:
  - improve errors thrown in header validation (Joyee Cheung) [#16718](https://github.com/nodej
s/node/pull/16718)

PR-URL: #17204
gibfahn added a commit that referenced this pull request Dec 5, 2017
Notable Changes:

- **console**:
  - avoid adding infinite error listeners (Matteo Collina) [#16770](https://github.com/nodejs/n
de/pull/16770)
- **http2**:
  - improve errors thrown in header validation (Joyee Cheung) [#16718](https://github.com/nodej
s/node/pull/16718)

PR-URL: #17204
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants