FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

console: avoid adding infinite error listeners by mcollina · Pull Request #16770 · nodejs/node · GitHub

/ node Public

console: avoid adding infinite error listeners - #16770

Closed
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:fix-16767
ConversationCommits (1)ChecksFiles changed
Closed

console: avoid adding infinite error listeners#16770
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:fix-16767

Conversation

mcollina commented Nov 5, 2017

Copy link
Copy Markdown
Member

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 added the console Issues and PRs related to the console subsystem. label Nov 5, 2017

mcollina commented Nov 5, 2017

Copy link
Copy Markdown
Member Author

@gibfahn this needs to go into node 8.

mscdex reviewed Nov 5, 2017
View reviewed changes
Comment thread test/parallel/test-console-log-stdio-broken-dest.js Outdated

mscdex Nov 5, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcollina Nov 5, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

updated

console: avoid adding infinite error listeners
8608334 
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 commented Nov 5, 2017

Copy link
Copy Markdown
Member Author

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

gibfahn commented Nov 5, 2017

Copy link
Copy Markdown
Member

@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 commented Nov 5, 2017

Copy link
Copy Markdown
Member

@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 commented Nov 5, 2017

Copy link
Copy Markdown
Member

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

addaleax commented Nov 5, 2017

Copy link
Copy Markdown
Member

@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 commented Nov 5, 2017

Copy link
Copy Markdown
Member Author

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

gibfahn commented Nov 5, 2017

Copy link
Copy Markdown
Member

@mcollina Tuesday, see #16783

mcollina commented Nov 6, 2017

Copy link
Copy Markdown
Member Author

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

mcollina commented Nov 6, 2017

Copy link
Copy Markdown
Member Author

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

gibfahn commented Nov 6, 2017

Copy link
Copy Markdown
Member

And after that?

Probably another couple of weeks.

sam-github commented Nov 6, 2017

Copy link
Copy Markdown
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 commented Nov 6, 2017

Copy link
Copy Markdown
Member

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 commented Nov 6, 2017

Copy link
Copy Markdown
Member Author

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.

sam-github reviewed Nov 6, 2017
View reviewed changes
Comment thread lib/console.js
// 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'

sam-github Nov 6, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mcollina commented Nov 8, 2017

Copy link
Copy Markdown
Member Author

Landed as d82bedc.

mcollina closed this Nov 8, 2017
mcollina deleted the fix-16767 branch November 8, 2017 11:45
mcollina added a commit that referenced this pull request Nov 8, 2017
console: avoid adding infinite error listeners
d82bedc 
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
gibfahn pushed a commit that referenced this pull request Nov 8, 2017
console: avoid adding infinite error listeners
9df1e8f 
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
console: avoid adding infinite error listeners
bed0560 
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 <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
evanlucas mentioned this pull request Nov 13, 2017
Merged
gibfahn mentioned this pull request Nov 21, 2017
Merged
gibfahn added a commit that referenced this pull request Dec 5, 2017
2017-12-05, Version 8.9.2 'Carbon' (LTS)
dcb2644 
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
2017-12-05, Version 8.9.2 'Carbon' (LTS)
92011e4 
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
+3 more reviewers
mscdex mscdex left review comments
sam-github sam-github approved these changes
apapirovski apapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

console Issues and PRs related to the console subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL