stream: improving error handling by mafintosh · Pull Request #18438 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation59 Commits15 Checks0 Files changed

Conversation

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]({{ revealButtonHref }})

mafintosh

Checklist
Affected core subsystem(s)

stream

This PR 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.

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

vsemozhetbyt
### ERR_STREAM_DESTROYED
An stream method was called that cannot complete cause the stream was been

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

@@ -106,6 +106,9 @@ function ReadableState(options, stream) {
this.readableListening = false;
this.resumeScheduled = false;
// True if the close was already emitted and should not emitted again

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

@@ -134,6 +134,9 @@ function WritableState(options, stream) {
// True if the error was already emitted and should not be thrown again
this.errorEmitted = false;
// True if the close was already emitted and should not emitted again

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

@jasnell

@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

Added docs about the close event and the ERR_STREAM_DESTROYED error.

@mcollina 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

// Set close emitted, so the stream destruction does not
// emit them
this._readableState.closeEmitted = true;
this._writableState.closeEmitted = true;

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

@@ -437,7 +441,8 @@ Readable.prototype.read = function(n) {
if (state.length === 0)
state.needReadable = true;
// call internal read method
this._read(state.highWaterMark);
if (!state.destroyed)

Choose a reason for hiding this comment

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

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

@@ -132,6 +132,8 @@ function Transform(options) {
}
function prefinish() {
if (this._readableState.destroyed)
return;

Choose a reason for hiding this comment

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

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?

@mcollina 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

mcollina

Choose a reason for hiding this comment

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

LGTM with a nit

@@ -219,6 +226,11 @@ function Socket(options) {
options = { fd: options }; // Legacy interface.
else if (options === undefined)
options = {};
else
options = copyObject(options);

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 mafintosh changed the titleImproving streams error handling stream: improving error handling

Jan 30, 2018

@mafintosh

@mcollina

@mafintosh can you add emitClose to the documented options?

@mafintosh

Trott
### ERR_STREAM_DESTROYED
A stream method was called that cannot complete cause the stream was been

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
### ERR_STREAM_DESTROYED
A stream method was called that cannot complete cause the stream was been

Choose a reason for hiding this comment

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

s/cause/because

@mafintosh

@mcollina

@mcollina

@mafintosh

@mcollina

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

@mcollina

mcollina pushed a commit that referenced this pull request

Mar 6, 2018

@mafintosh @mcollina

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

@mafintosh mafintosh deleted the streams-error-handling-fix branch

March 6, 2018 12:57

@joyeecheung

mcollina added a commit to mcollina/node that referenced this pull request

Mar 6, 2018

@mcollina

mcollina added a commit that referenced this pull request

Mar 6, 2018

@mcollina

See: #18438

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

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request

May 8, 2018

@mafintosh @MayaLekova

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

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request

May 8, 2018

@mcollina @MayaLekova

See: nodejs#18438

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

@kjin kjin mentioned this pull request

Sep 11, 2018

3 tasks

mcollina added a commit to mcollina/node that referenced this pull request

Jan 10, 2019

@mcollina

mcollina added a commit that referenced this pull request

Jan 12, 2019

@mcollina

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

addaleax pushed a commit that referenced this pull request

Jan 14, 2019

@mcollina @addaleax

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

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Jan 16, 2019

@mcollina @BridgeAR

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

BethGriggs pushed a commit that referenced this pull request

Apr 28, 2019

@mcollina @BethGriggs

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

BethGriggs pushed a commit that referenced this pull request

May 10, 2019

@mcollina @BethGriggs

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

MylesBorins pushed a commit that referenced this pull request

May 16, 2019

@mcollina @MylesBorins

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

Reviewers

@Trott Trott Trott left review comments

@lpinca lpinca lpinca left review comments

@cjihrig cjihrig cjihrig left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

Labels

fs

Issues and PRs related to the fs subsystem / file system.

http2

Issues or PRs related to the http2 subsystem.

semver-major

PRs that contain breaking changes and should be released in the next major version.

stream

Issues and PRs related to the stream subsystem.