Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: avoid memory leaks when writable.write() return false #10631

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions doc/api/stream.md
Original file line number Diff line number Diff line change
Expand Up @@ -443,9 +443,46 @@ first argument. To reliably detect write errors, add a listener for the
The return value is `true` if the internal buffer does not exceed
`highWaterMark` configured when the stream was created after admitting `chunk`.
If `false` is returned, further attempts to write data to the stream should
stop until the [`'drain'`][] event is emitted. However, the `false` return
value is only advisory and the writable stream will unconditionally accept and
buffer `chunk` even if it has not not been allowed to drain.
stop until the [`'drain'`][] event is emitted.

While a stream is not draining, calls to `write()` will buffer `chunk`, and
return false. Once all currently buffered chunks are drained (accepted for
delivery by the operating system), the `'drain'` event will be emitted.
It is recommended that once write() returns false, no more chunks be written
until the `'drain'` event is emitted. While calling `write()` on a stream that
is not draining is allowed, Node.js will buffer all written chunks until
maximum memory usage occurs, at which point it will abort unconditionally.
Even before it aborts, high memory usage will cause poor garbage collector
performance and high RSS (which is not typically released back to the system,
even after the memory is no longer required). Since TCP sockets may never
drain if the remote peer does not read the data, writing a socket that is
not draining may lead to a remotely exploitable vulnerability.

Writing data while the stream is not draining is particularly
problematic for a [Transform][], because the `Transform` streams are paused
by default until they are piped or an `'data'` or `'readable'` event handler
is added.

If the data to be written can be generated or fetched on demand, it is
recommended to encapsulate the logic into a [Readable][] and use
[`stream.pipe()`][]. However, if calling `write()` is preferred, it is
possible to respect backpressure and avoid memory issues using the
the [`'drain'`][] event:

```js
function write (data, cb) {
if (!stream.write(data)) {
stream.once('drain', cb)
} else {
process.nextTick(cb)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth mentioning that code like the above is a streams anti-pattern? If I saw code like this, I would suggest that the source of the data should be restructured as a "readable stream", and that the readable stream should then be piped to the destination. Even if its not an anti-pattenr, I think its worth at least mentioning that the above code is not necessary when piping streams together, and that restructuring your app as piped streams allows back pressure to be more elegantly handled by node directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree that it is an anti-pattern. The major difference is speed, this runs at 2-3x compared to a Readable. However, I agree that applications should built around pipe() rather than write() 100%.

here is the problem: if someone does 3-4 writes without checking 'drain' is never an issue. However, if them wants to write some megabytes of data, then doing this pattern might be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, and that experience with streams is really valuable. I think you are (quite!) understandably trying to avoid writing a guide on streams and backpressure, while still providing some useful guidance inline in the docs, but I'm concerned that without a little more context people will paste your code into their app, and just move the leak around. No pressure to do a full-scale guide, :-), I'm not -1 on the PR, its an incremental improvement.


// Wait for cb to be called before doing any other write.
write('hello', () => {
console.log('write completed, do more writes now')
})
```

A Writable stream in object mode will always ignore the `encoding` argument.

Expand Down