stream: improving error handling by mafintosh · Pull Request #18438 · nodejs/node

mafintosh · 2018-01-29T18:58:09Z

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)

stream

This PR improves error handling for streams in a few ways.

It ensures that no user defined methods (_read, _write, ...) are run after .destroy has been called.
It introduces an explicit error to tell the user if they are write to write, etc to the stream after it has been destroyed.
It makes streams always emit close as the last thing after they have been destroyed
Changes the default _destroy to not gracefully end streams.

Especially 4. makes it easier for userland modules to handle stream errors as they don't appear as graceful closes anymore. Seen issues being with end-of-stream/pump because of this.

mafintosh · 2018-01-29T18:59:23Z

/cc @mcollina @nodejs/streams

vsemozhetbyt · 2018-01-29T19:04:47Z

Nits:
An stream -> A stream
was been -> has been

vsemozhetbyt · 2018-01-29T19:06:18Z

A nit: should not emitted -> should not be emitted

vsemozhetbyt · 2018-01-29T19:06:46Z

A nit: should not emitted -> should not be emitted

jasnell · 2018-01-29T19:31:31Z

@nodejs/http2 ... just a quick note that this will have a relatively small impact on the 'close' event currently emitted for Http2Stream and Http2Session.

mafintosh · 2018-01-30T10:19:23Z

Added docs about the close event and the ERR_STREAM_DESTROYED error.

mcollina · 2018-01-30T11:12:04Z

I'm not very fond of this lines. A current goal is to reduce (or totally remove) the number of access to _readableState and _writableState. However, this requires it. Maybe emitting 'close' automatically can be moved to an option?

This is only because http2 currently emits a 'code' argument with the close event. The idea is to get rid of that in http2 and move the 'code' to a property down the line, but this ensures backwards compat right now.
Should I add a comment explaining that?

What I mean is that in both fs and http2 for compat reason we need to not emit 'close' on destroy. I think this might actually be a legit use case, and so we should support it with an option at construction.

The fs thing is legit, but we should just get rid of the code in http2 imo, but that's another discussion. I like the idea of an option, adding it.

mcollina · 2018-01-30T11:13:34Z

I think this check can me moved in the previous if block: state.ended || state.reading || state.destroyed.

mcollina · 2018-01-30T11:14:49Z

can you add one more test for the _flush() logic and destroy()? It seems you are fixing a bug here. Or without this change some tests would not pass anymore?

mafintosh · 2018-01-30T16:58:02Z

@mcollina fixed your comments

mcollina

LGTM with a nit

mcollina · 2018-01-30T17:30:12Z

You can use Object.assign.

mafintosh · 2018-01-30T19:36:13Z

@mcollina nit fixed

mcollina · 2018-01-31T07:54:30Z

@mafintosh can you add emitClose to the documented options?

mafintosh · 2018-01-31T09:34:07Z

@mcollina fixed the docs

Trott · 2018-01-31T19:13:13Z

Seems like cause should be because here, I think?

jasnell · 2018-01-31T19:21:26Z

s/cause/because

mafintosh · 2018-01-31T20:59:49Z

@jasnell @Trott fixed

mcollina · 2018-02-01T12:00:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12879/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1250/

mcollina · 2018-02-01T12:38:36Z

CI (Again): https://ci.nodejs.org/job/node-test-pull-request/12881/

mafintosh · 2018-02-02T10:12:43Z

@mcollina ci seemed to have an issue?

mcollina · 2018-02-02T22:30:04Z

We are testing in so many system that a botched run happens every now and then.

mcollina · 2018-03-06T12:31:25Z

Landed as 5e3f516

This improves error handling for streams in a few ways. 1. It ensures that no user defined methods (_read, _write, ...) are run after .destroy has been called. 2. It introduces an explicit error to tell the user if they are write to write, etc to the stream after it has been destroyed. 3. It makes streams always emit close as the last thing after they have been destroyed 4. Changes the default _destroy to not gracefully end streams. It also updates net, http2, zlib and fs to the new error handling. PR-URL: #18438 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

joyeecheung · 2018-03-06T13:06:44Z

This is breaking the linter on master

07:52:11 doc/api/stream.md
07:52:11 1392:84 warning Line must be at most 80 characters maximum-line-length remark-lint

https://ci.nodejs.org/job/node-test-linter/16731/console

See: nodejs#18438

See: #18438 PR-URL: #19169 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This improves error handling for streams in a few ways. 1. It ensures that no user defined methods (_read, _write, ...) are run after .destroy has been called. 2. It introduces an explicit error to tell the user if they are write to write, etc to the stream after it has been destroyed. 3. It makes streams always emit close as the last thing after they have been destroyed 4. Changes the default _destroy to not gracefully end streams. It also updates net, http2, zlib and fs to the new error handling. PR-URL: nodejs#18438 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

See: nodejs#18438 PR-URL: nodejs#19169 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

See: nodejs#25373 See: nodejs#18438

See: #25373 See: #18438 PR-URL: #25413 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

See: nodejs#25373 See: nodejs#18438 PR-URL: nodejs#25413 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

See: #25373 See: #18438 PR-URL: #25413 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 29, 2018

vsemozhetbyt reviewed Jan 29, 2018
View reviewed changes

Comment threaddoc/api/errors.md Outdated
vsemozhetbytJan 29, 2018
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.
Nits:
An stream -> A stream
was been -> has been

vsemozhetbyt reviewed Jan 29, 2018
View reviewed changes

Comment threadlib/_stream_readable.js Outdated
vsemozhetbytJan 29, 2018
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.
A nit: should not emitted -> should not be emitted

vsemozhetbyt reviewed Jan 29, 2018
View reviewed changes

Comment threadlib/_stream_writable.js Outdated
vsemozhetbytJan 29, 2018
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.
A nit: should not emitted -> should not be emitted

mcollina added stream Issues and PRs related to the stream subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 30, 2018

mcollina reviewed Jan 30, 2018
View reviewed changes

mcollina added fs Issues and PRs related to the fs subsystem / file system. http2 Issues or PRs related to the http2 subsystem. labels Jan 30, 2018

mafintosh force-pushed the streams-error-handling-fix branch from 4bc9c16 to 958d5beCompare January 30, 2018 12:40

mcollina approved these changes Jan 30, 2018
View reviewed changes

Comment threadlib/net.js Outdated
mcollinaJan 30, 2018
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use Object.assign.

mafintosh changed the title ~~Improving streams error handling~~stream: improving error handlingJan 30, 2018

mcollina requested a review from a teamJanuary 31, 2018 07:55

Trott reviewed Jan 31, 2018
View reviewed changes

Comment threaddoc/api/errors.md Outdated
TrottJan 31, 2018
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like cause should be because here, I think?

jasnell reviewed Jan 31, 2018
View reviewed changes

Comment threaddoc/api/errors.md Outdated
jasnellJan 31, 2018
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/cause/because

mcollina requested a review from a teamFebruary 2, 2018 22:30

BridgeAR requested a review from jasnell February 6, 2018 22:03

mafintosh force-pushed the streams-error-handling-fix branch from 6e65b5e to 1689bafCompare March 6, 2018 11:19

mcollina closed this Mar 6, 2018

mafintosh deleted the streams-error-handling-fix branch March 6, 2018 12:57

mcollina added a commit to mcollina/node that referenced this pull request Mar 6, 2018
doc: fix max length on stream.md
fff2c8b
See: nodejs#18438

mcollina mentioned this pull request Mar 6, 2018
doc: fix max length on stream.md #19169
Closed
2 tasks

mcollina added a commit that referenced this pull request Mar 6, 2018
doc: fix max length on stream.md
1a5ec83
See: #18438 PR-URL: #19169 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

targos mentioned this pull request Apr 2, 2018
http2: eliminate unnecessary nextTicks, use stream close event #19451
Closed
4 tasks

mafintosh mentioned this pull request Apr 5, 2018
stream: always emit error before close #19836
Closed
4 tasks

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
doc: fix max length on stream.md
adc9ba4
See: nodejs#18438 PR-URL: nodejs#19169 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

ChALkeR mentioned this pull request Jun 25, 2018
doc: remove 2 unused error codes from errors.md #21491
Merged
4 tasks

kjin mentioned this pull request Sep 11, 2018
http2: fix responses to long payload reqs #20084
Closed
3 tasks

addaleax mentioned this pull request Oct 20, 2018
http2: simplify timeout tracking #19206
Closed
2 tasks

MylesBorins mentioned this pull request Nov 29, 2018
http2: [8.x] server stream emits error after close #24559
Closed

mcollina mentioned this pull request Jan 9, 2019
Inconsistent stream events using destroy method #25373
Closed

mcollina mentioned this pull request Jan 9, 2019
docs: document that stream.on('close') was changed in Node 10. #25413
Closed
3 tasks

mcollina added a commit to mcollina/node that referenced this pull request Jan 10, 2019
doc: document that stream.on('close') was changed in Node 10
401ca19
See: nodejs#25373 See: nodejs#18438

fijimunkii mentioned this pull request Mar 25, 2019
More errors fijimunkii/node-proxyproto#1
Merged

mcollina mentioned this pull request May 7, 2020
http: added scheduling option to http agent #33278
Closed
4 tasks

Uh oh!

Uh oh!

Conversation

mafintosh commented Jan 29, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

mafintosh commented Jan 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 29, 2018

Uh oh!

mafintosh commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafintosh commented Jan 30, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafintosh commented Jan 30, 2018

Uh oh!

mcollina commented Jan 31, 2018

Uh oh!

mafintosh commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafintosh commented Jan 31, 2018

Uh oh!

mcollina commented Feb 1, 2018

Uh oh!

mcollina commented Feb 1, 2018

Uh oh!

mafintosh commented Feb 2, 2018

Uh oh!

mcollina commented Feb 2, 2018

Uh oh!

mcollina commented Mar 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Mar 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

mafintosh commented Jan 29, 2018•
edited
Loading

mcollina commented Mar 6, 2018•
edited
Loading