Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upstream: improving error handling #18438
Conversation
nodejs-github-bot
added
the
lib / src
label
Jan 29, 2018
This comment has been minimized.
This comment has been minimized.
vsemozhetbyt
reviewed
Jan 29, 2018
| <a id="ERR_STREAM_DESTROYED"></a> | ||
| ### ERR_STREAM_DESTROYED | ||
|
|
||
| An stream method was called that cannot complete cause the stream was been |
This comment has been minimized.
This comment has been minimized.
vsemozhetbyt
reviewed
Jan 29, 2018
| @@ -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 | |||
This comment has been minimized.
This comment has been minimized.
vsemozhetbyt
reviewed
Jan 29, 2018
| @@ -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 | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@nodejs/http2 ... just a quick note that this will have a relatively small impact on the |
This comment has been minimized.
This comment has been minimized.
|
Added docs about the |
mcollina
added
stream
semver-major
and removed
lib / src
labels
Jan 30, 2018
mcollina
reviewed
Jan 30, 2018
| // Set close emitted, so the stream destruction does not | ||
| // emit them | ||
| this._readableState.closeEmitted = true; | ||
| this._writableState.closeEmitted = true; |
This comment has been minimized.
This comment has been minimized.
mcollina
Jan 30, 2018
Member
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 comment has been minimized.
This comment has been minimized.
mafintosh
Jan 30, 2018
Author
Member
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?
This comment has been minimized.
This comment has been minimized.
mcollina
Jan 30, 2018
Member
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.
This comment has been minimized.
This comment has been minimized.
mafintosh
Jan 30, 2018
Author
Member
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) | |||
This comment has been minimized.
This comment has been minimized.
mcollina
Jan 30, 2018
Member
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; | |||
This comment has been minimized.
This comment has been minimized.
mcollina
Jan 30, 2018
Member
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
added
fs
http2
labels
Jan 30, 2018
mafintosh
force-pushed the
mafintosh:streams-error-handling-fix
branch
from
4bc9c16
to
958d5be
Jan 30, 2018
This comment has been minimized.
This comment has been minimized.
|
@mcollina fixed your comments |
mcollina
approved these changes
Jan 30, 2018
|
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); | |||
This comment has been minimized.
This comment has been minimized.
mafintosh
changed the title
Improving streams error handling
stream: improving error handling
Jan 30, 2018
This comment has been minimized.
This comment has been minimized.
|
@mcollina nit fixed |
This comment has been minimized.
This comment has been minimized.
|
@mafintosh can you add |
mcollina
requested a review
from nodejs/tsc
Jan 31, 2018
This comment has been minimized.
This comment has been minimized.
|
@mcollina fixed the docs |
Trott
reviewed
Jan 31, 2018
| <a id="ERR_STREAM_DESTROYED"></a> | ||
| ### ERR_STREAM_DESTROYED | ||
|
|
||
| A stream method was called that cannot complete cause the stream was been |
This comment has been minimized.
This comment has been minimized.
jasnell
reviewed
Jan 31, 2018
| <a id="ERR_STREAM_DESTROYED"></a> | ||
| ### ERR_STREAM_DESTROYED | ||
|
|
||
| A stream method was called that cannot complete cause the stream was been |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@mcollina ci seemed to have an issue? |
This comment has been minimized.
This comment has been minimized.
|
We are testing in so many system that a botched run happens every now and then. |
mcollina
requested a review
from nodejs/tsc
Feb 2, 2018
BridgeAR
requested a review
from
jasnell
Feb 6, 2018
mafintosh
added some commits
Mar 5, 2018
mafintosh
force-pushed the
mafintosh:streams-error-handling-fix
branch
from
6e65b5e
to
1689baf
Mar 6, 2018
This comment has been minimized.
This comment has been minimized.
|
Landed as 5e3f516 |
mcollina
closed this
Mar 6, 2018
mcollina
added a commit
that referenced
this pull request
Mar 6, 2018
mafintosh
deleted the
mafintosh:streams-error-handling-fix
branch
Mar 6, 2018
This comment has been minimized.
This comment has been minimized.
|
This is breaking the linter on master
|
mafintosh commentedJan 29, 2018
•
edited
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
stream
This PR improves error handling for streams in a few ways.
closeas the last thing after they have been destroyedEspecially 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.