doc: add comments about Readable unpipe on Writable error event in stream.md #18080

GeorgeSapkin · 2018-01-10T15:08:23Z

Documenting Readable unpiping behavior according to: https://github.com/nodejs/node/blob/v8.x/lib/_stream_readable.js#L656-L664

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

addaleax · 2018-01-10T15:15:17Z

Hi, thanks for the PR! You edited the section for stream implementors that tells them how to report errors; I think this would be better kept in the section about .pipe() itself? Also, it might not be clear from the current wording on which of the two streams emitting the error event stops piping.

GeorgeSapkin · 2018-01-10T15:28:52Z

Do you think it should be in Readable.pipe() section? It seems to me it affects the Writable stream implementers more since right now it is not clear what happens when Writable emits errors. Maybe it should be in both or there should some kind of a reference between the sections?

I've improved the wording to make it clearer which stream emits an error.

Trott · 2018-01-10T21:43:06Z

doc/api/stream.md

@@ -1508,6 +1508,9 @@ the callback and passing the error as the first argument. This will cause an
 on how the stream is being used.  Using the callback ensures consistent and
 predictable handling of errors.

+*Note*: If a Readable stream pipes into a Writable stream when Writable emits an


Nit: Consider removing *Note*: and letting the sentence stand on its own?

This seems like something that requires special attention when implementing error handling in Writable streams and STYLE_GUIDE.md recommends to mark such things with a *Note*:.

That should probably be updated as well. We try to minimize those because they became to frequent and therefore lost there meaning.

I've removed the *Note*.

Trott · 2018-01-10T21:43:33Z

doc/api/stream.md

@@ -1508,6 +1508,9 @@ the callback and passing the error as the first argument. This will cause an
 on how the stream is being used.  Using the callback ensures consistent and
 predictable handling of errors.

+*Note*: If a Readable stream pipes into a Writable stream when Writable emits an
+error, Readable stream will be unpiped.


Nit: add the before Readable stream.

BridgeAR · 2018-02-01T17:52:17Z

I agree with @addaleax that it should be moved to the pipe part.

See https://github.com/GeorgeSapkin/node/blob/3c0aa8a6bae0e27c988bef175cbd31ddbcb4ecef/doc/api/stream.md#event-pipe

GeorgeSapkin · 2018-02-05T11:19:45Z

@BridgeAR why would this be in the Writable pipe event's section (judging from your link)? Did you mean error or unpipe?

BridgeAR · 2018-02-05T11:58:11Z

@GeorgeSapkin yes unpipe is probably the correct one in this case. But it should be reworded in that case.

Something like e.g. might fit?

This will also be called in case this Writable stream emits an error.

@mcollina PTAL

mcollina

LGTM

mcollina · 2018-02-06T09:04:47Z

I think we should really add the sentence in 2-3 places. It's one of the most forgotten (and bad) features of streams.

I would also note that calling unpipe() means the stream does not get closed.

GeorgeSapkin · 2018-02-08T08:24:26Z

@BridgeAR @mcollina I've added another remark in the Writable unpipe event's sections.

mcollina · 2018-02-08T11:20:29Z

Can you add a sentence about the fact that calling unpipe() means the stream does not get closed.

BridgeAR · 2018-02-13T05:33:27Z

Ping @GeorgeSapkin

GeorgeSapkin · 2018-02-13T12:48:16Z

@mcollina I'm not sure I understand where the note about streams not being closed on unpipe needs to be added and why that (i.e. automatic closing of a stream) would be assumed to be the case to begin with.

mcollina · 2018-02-13T14:20:42Z

@GeorgeSapkin the main reason is that a stream is typically holding on to some native resources. A file descriptor, or some other native data. If an error happen and the destination stream is unpiped, the underlining native resource is not freed, and will trigger a memory (or file descriptor) leak.

BridgeAR · 2018-03-02T04:07:15Z

I would say we can land this as is (it already improves the current situation) and we have a follow up PR that addresses your comment @mcollina ?

mcollina · 2018-03-02T08:56:21Z

👍

mcollina · 2018-03-02T08:56:30Z

~~landing~~

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

mcollina · 2018-03-02T15:12:50Z

Landed in 1d19fd3

PR-URL: #18080 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#18080 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #22380 PR-URL: #18080 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jan 10, 2018

Trott reviewed Jan 10, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

GeorgeSapkin changed the title ~~doc: add Note about Readable unpipe on Writable error event in stream.md~~ doc: add a comment about Readable unpipe on Writable error event in stream.md Feb 5, 2018

mcollina approved these changes Feb 6, 2018

View reviewed changes

doc: add comments about Readable unpipe on Writable error event

29fc4f2

GeorgeSapkin changed the title ~~doc: add a comment about Readable unpipe on Writable error event in stream.md~~ doc: add comments about Readable unpipe on Writable error event in stream.md Feb 8, 2018

jasnell approved these changes Feb 9, 2018

View reviewed changes

mcollina closed this Mar 2, 2018

mcollina pushed a commit that referenced this pull request Mar 2, 2018

doc: Readable unpipe on Writable error event

1d19fd3

PR-URL: #18080 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit to addaleax/node that referenced this pull request Mar 5, 2018

doc: Readable unpipe on Writable error event

459f209

PR-URL: nodejs#18080 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Mar 6, 2018

v9.8.0 proposal #19181

Merged

GeorgeSapkin deleted the stream-unpipe-doc branch March 29, 2018 10:57

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

doc: Readable unpipe on Writable error event

e2dc2bd

PR-URL: nodejs#18080 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell pushed a commit to jasnell/node that referenced this pull request Aug 17, 2018

doc: Readable unpipe on Writable error event

3f0973f

PR-URL: nodejs#18080 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Sep 6, 2018

doc: Readable unpipe on Writable error event

7f6e0b3

Backport-PR-URL: #22380 PR-URL: #18080 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Sep 6, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add comments about Readable unpipe on Writable error event in stream.md #18080

doc: add comments about Readable unpipe on Writable error event in stream.md #18080

GeorgeSapkin commented Jan 10, 2018

addaleax commented Jan 10, 2018

GeorgeSapkin commented Jan 10, 2018

Trott Jan 10, 2018

GeorgeSapkin Jan 10, 2018

BridgeAR Feb 1, 2018

GeorgeSapkin Feb 5, 2018

Trott Jan 10, 2018

GeorgeSapkin Jan 10, 2018

BridgeAR commented Feb 1, 2018

GeorgeSapkin commented Feb 5, 2018

BridgeAR commented Feb 5, 2018

mcollina left a comment

mcollina commented Feb 6, 2018

GeorgeSapkin commented Feb 8, 2018

mcollina commented Feb 8, 2018

BridgeAR commented Feb 13, 2018

GeorgeSapkin commented Feb 13, 2018

mcollina commented Feb 13, 2018

BridgeAR commented Mar 2, 2018

mcollina commented Mar 2, 2018

mcollina commented Mar 2, 2018 •

edited

Loading

mcollina commented Mar 2, 2018

doc: add comments about Readable unpipe on Writable error event in stream.md #18080

doc: add comments about Readable unpipe on Writable error event in stream.md #18080

Conversation

GeorgeSapkin commented Jan 10, 2018

Checklist

Affected core subsystem(s)

addaleax commented Jan 10, 2018

GeorgeSapkin commented Jan 10, 2018

Trott Jan 10, 2018

Choose a reason for hiding this comment

GeorgeSapkin Jan 10, 2018

Choose a reason for hiding this comment

BridgeAR Feb 1, 2018

Choose a reason for hiding this comment

GeorgeSapkin Feb 5, 2018

Choose a reason for hiding this comment

Trott Jan 10, 2018

Choose a reason for hiding this comment

GeorgeSapkin Jan 10, 2018

Choose a reason for hiding this comment

BridgeAR commented Feb 1, 2018

GeorgeSapkin commented Feb 5, 2018

BridgeAR commented Feb 5, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Feb 6, 2018

GeorgeSapkin commented Feb 8, 2018

mcollina commented Feb 8, 2018

BridgeAR commented Feb 13, 2018

GeorgeSapkin commented Feb 13, 2018

mcollina commented Feb 13, 2018

BridgeAR commented Mar 2, 2018

mcollina commented Mar 2, 2018

mcollina commented Mar 2, 2018 • edited Loading

mcollina commented Mar 2, 2018

mcollina commented Mar 2, 2018 •

edited

Loading