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

[GeorgeSapkin]

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]

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]

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]

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

[GeorgeSapkin]

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*:.

[BridgeAR]

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

[GeorgeSapkin]

I've removed the *Note*.

[Trott]

Nit: add the before Readable stream.

[GeorgeSapkin]

Done.

[BridgeAR]

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]

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

[BridgeAR]

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

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]

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

[mcollina]

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

[BridgeAR]

Ping @GeorgeSapkin

[GeorgeSapkin]

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

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

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]

👍

[mcollina]

landing

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

[mcollina]

Landed in 1d19fd3