-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
Conversation
448fb65
to
9aababd
Compare
In case the `false` return value is ignored, the writable stream will | ||
unconditionally accept and buffer `chunk` even if it has not not been allowed | ||
to drain. However, this might lead to memory leaks and garbage | ||
collection issues, especially if the writable is a [Transform]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be [Transform][]
? for linking?
This may be more readable if like:
especially if the writable stream is a [Transform]()
or even especially if the stream is a [Transform]()
?
unconditionally accept and buffer `chunk` even if it has not not been allowed | ||
to drain. However, this might lead to memory leaks and garbage | ||
collection issues, especially if the writable is a [Transform]() | ||
(which inherit from [Writable]()). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be inherits
?
Also, same question about the linking of [Writable]()
here
if (!stream.write(data)) { | ||
stream.once('drain', cb) | ||
} else { | ||
cb() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we encourage consistency of async vs sync calling of the callback? Maybe wrap in setImmediate()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll wrap in a process.nextTick()
, as there is no need to slow things down doing a full I/O cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough :]
9aababd
to
7f43650
Compare
@evanlucas thanks, updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a comment that should be addressed before landing. Thanks!
collection issues, especially if the writable is a [Transform][] | ||
(which inherits from [Writable][]). | ||
|
||
A possible way to handle the [`'drain'``][] event to respect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the drain link has an extra backtick at the end which will break the link.
7f43650
to
f03a56e
Compare
|
||
In case the `false` return value is ignored, the writable stream will | ||
unconditionally accept and buffer `chunk` even if it has not not been allowed | ||
to drain. However, this might lead to memory leaks and garbage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be called a leak, but better to describe that node has limited memory to buffer data that it is not ready to write, and will abort when it runs out of memory. Leak is vague, but more usually applies to memory your application doesn't need, but still refs. In this case, the app needs the write buffers until they get written, they aren't leaking.
Also, why is this especially true for Transform streams? If we are going to say that its more true for particular derived types of Writable, we should say why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why is this especially true for Transform streams? If we are going to say that its more true for particular derived types of Writable, we should say why.
A Writable
in Node core is always writing thing to the underlining resource, e.g. the socket. You can stop that with cork()
, but by default it flows the data. Transform
things are paused by default until they are piped or an 'data'
or 'readable'
event handler is added.
So, ignoring when write()
returns false will cause the data to be buffered until the stream is flowing. If that does not happen, you have a memory leak.
Should I add the above to the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I was trying to say in the conversation that sparked this PR. If your code leaks when write()
returns false
, it is still leaking when write()
returns true
. And it is the stream being written to that is doing the leaking. If write()
returns false
and never drains itself or you call end()
after write()
returns true
and it never drains its buffer, that’s the same sort of leak (though if the target stream treats end()
specially, you might only see the leak happen when the target stream fails to drain after write()
returns false
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@binki under your definition, a Transform
stream that is not piped is a memory leak, which is the point I am trying to make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be called a leak, but better to describe that node has limited memory to buffer data that it is not ready to write, and will abort when it runs out of memory. Leak is vague, but more usually applies to memory your application doesn't need, but still refs. In this case, the app needs the write buffers until they get written, they aren't leaking.
It is a memory leak in behavior, but if it's a contested term I'll remove it. As long as the docs is clear that you are likely to get aborted by out of memory if you don't handle drain, then I'm good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina I think the info on the Transform stream you put above is a subtle gotcha, well worth documenting. It should be mentioned here, and maybe in the docs for Transform as well? This spreading around is why a guide would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, another behavior that usually happens in this case is a big RSS with low heap usage. A single stream ballooned the memory usage of the process, but in fact all of that is free. It has been collected, but the process is left big. Most people will flag this behavior as a memory leak, even if it is not in the strict definition of a chunk of memory that is left allocated but it is not referenced by anybody.
How about we define this as a "memory handling issues" or "memory ballooning"?
Its out of scope of this PR, but node needs a guide on "Backpressure", describing the problems it solves (like indefinite buffering causing memory exhaustion and excessive drag on the GC), and what patterns must be respected for an app to utilize it (streams, avoiding use of .write(), and if you are using .write(), respecting its return value). This would be better than mini tutorials spread the through the docs, IMO. |
@sam-github I think we should have a topic/guide/whatever on streams. It is probably a big chunk of work on its own. |
f03a56e
to
8348219
Compare
@sam-github @binki I have updated the PR, let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think wait for 'drain'
instead of listen for 'drain'
would be much clearer, my other comments can be ignored.
In case the `false` return value is ignored, the writable stream will | ||
unconditionally accept and buffer `chunk` even if it has not not been allowed | ||
to drain. However, Node has a limited amount of memory, and not | ||
listening to `'drain'` might cause the process to abort by an out of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better!
However, I think it would be clearer if you replace listening to `'drain'`
with waiting for `'drain'`
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina I would like to suggest this alternate text to replace from If false is returned
to such as high RSS.
:
While a stream is not draining, calls to
write()
will bufferchunk
, and returnfalse
. Once all currently buffered chunks are drained (accepted for delivery by the operating system), the'drain'
event will be emitted. It is recommended that oncewrite()
returns false, no more chunks be written until the'drain'
event is emitted. While callingwrite()
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.
because the `Transform` streams are paused by default until | ||
they are piped or an `'data'` or `'readable'` event handler is added. | ||
So, ignoring when `write()` returns false will cause the data to be buffered | ||
until the stream is flowing, causing a memory leak. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose Transform
streams cannot flow until they are connected to consumers, but the same issues exist even for writable FS/network streams because you never know when they’ll start draining. Especially if the other end is a TCP server, the TCP server could just never read anything from its end and cause the exact same problem once the OS-level sockets buffers fill up. So I’m not convinced this is actually a Transform
-specific thing, but I think I’m alone with that opinion. Also, it’s not a memory leak unless you know for certain that the Transform
stream will never be piped to something else in the future. E.g., you could be buffering an entire file in-memory on purpose by using a Transform
stream with the identity transform ;-).
I don’t understand the RSS issue, though, does expanding a Buffer
and then letting it get GCed at a later point really cause node
to allocate memory without ever freeing it? Again, my concerns are probably dismissable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it’s not a memory leak unless you know for certain that the Transform stream will never be piped to something else in the future. E.g., you could be buffering an entire file in-memory on purpose by using a Transform stream with the identity transform ;-).
Even if it can be done, it does not mean it should be done. That type of use causes a huge lot of problems. I've amended things slightly, clarifying more.
I don’t understand the RSS issue, though, does expanding a Buffer and then letting it get GCed at a later point really cause node to allocate memory without ever freeing it? Again, my concerns are probably dismissable.
Buffers cannot be enlarged, but the heap on which are stored can. No, after it grows it does not go back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand the heap does shrink when the memory pressure goes down, but it would be delayed as shrinking the heap(usually by compacting the old space) would stop-the-world, so under some kind of workload it might never find a good time to shrink.
EDIT: as I understand Buffer's backing store memory are freed when the weak callbacks are called, and that would be delayed too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joyeecheung in my experience in debugging those issues in production, the shrinking does not happen in a reasonable timeframe, i.e. before causing other issues. In those cases memory occupation is stable at around 900MB-1GB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina Yes that's what I've observed too. Usually this would not lead to an OOM because there is always the "last resort gc" that would try super hard to clean up everything.
...but that's kinda off topic for this PR. If I am being too talky, just ignore me :D
process.nextTick(cb) | ||
} | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what you’re suggesting is to actually write code as if you should always wait for drain. This function is just a utility to emulate a drain happening even when one won’t happen by providing an asynchronous alternative to write()
which completes when stream
is no longer trying to exert backpressure. I think a short explanation of how this solution only works if you serialize your write()
calls to wait for the cb
to be called would be helpful to clarify how this is meant to help. E.g., to respect backpressure is to use the following. You must always wait for
cbto be called before calling this
write() wrapper again to respect backpressure.
Hmm, maybe that just makes it noisier.
This suggestion does simplify the code needed to respect 'drain'
by moving the part dealing with write()
’s return value behind an “always wait for target to be ready” API. But it would still, depending on where the write()
calls are coming from, require significant changes in the code that was originally calling write()
to use correctly. But if the code you start with is just a series of calls to write()
this would be straightforward enough and is rather nifty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that a small example is worth more than words, but that's me. Is the example clear enough in the context of this document, or is not and it needs some more words nearby?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina It might be clear enough if people know that they have to wait for cb
to be called before calling it again. I’m just not sure if that’s obvious, it wasn’t to me for some reason. Maybe an example of how to call it would have made it clearer to me more quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an example and a comment.
906df5f
to
8f7ec07
Compare
@sam-github are you ok? may I merge? |
memory error, and other types of memory and garbage collection | ||
issues, such as high RSS. | ||
|
||
The `'drain'` event must be handled if the writable is a [Transform][], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not true that it must be handled, can you rephrase as "Writing data while the stream is not draining is particularly problematic for a [Transform][], ..."
} else { | ||
process.nextTick(cb) | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize sentence, end it with a period, and wrap to 80 columns.
// wait for cb to be called before doing | ||
// any other write | ||
write('hello', () => { | ||
write('world', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, instead of the string 'world' being buffered in the stream, it is being buffered in a closure, pushing the memory leak out of the stream and closer to whatever process is producing the data to be written. Anybody who understand backpressure will understand that, but they wouldn't need the example in the first place. In the absence of a guide on backpressure, though, I guess this the best we can do, unless @mcollina you can think of some text to give guidance on when this pattern is reasonable to use, as opposed to using streams?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example of something that could be backpressured safely using this pattern is anything that can be calculated on demand. E.g., a sequence of numbers, consuming a generator, or producing x
zeroed out bytes. You could close over the necessary state. But these all would also be better wrapped as Readable
streams anyway as suggested above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the example because it was asked for in a review. I am happy in removing it.
5ca64e1
to
e8d4dd6
Compare
@sam-github I have made some changes, please review. |
e8d4dd6
to
46ea4db
Compare
@sam-github I made more edits. It should be good now, and much more clear than before. |
The doc specified that writable.write() was advisory only. However, ignoring that value might lead to memory leaks. This PR specifies that behavior. Moreover, it adds an example on how to listen for the 'drain' event correctly. See: nodejs@f347dad
46ea4db
to
e354d5c
Compare
Landed in e9044c8. |
The doc specified that writable.write() was advisory only. However, ignoring that value might lead to memory leaks. This PR specifies that behavior. Moreover, it adds an example on how to listen for the 'drain' event correctly. See: f347dad PR-URL: #10631 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
The doc specified that writable.write() was advisory only. However, ignoring that value might lead to memory leaks. This PR specifies that behavior. Moreover, it adds an example on how to listen for the 'drain' event correctly. See: nodejs@f347dad PR-URL: nodejs#10631 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
The doc specified that writable.write() was advisory only. However, ignoring that value might lead to memory leaks. This PR specifies that behavior. Moreover, it adds an example on how to listen for the 'drain' event correctly. See: nodejs@f347dad PR-URL: nodejs#10631 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
The doc specified that writable.write() was advisory only. However, ignoring that value might lead to memory leaks. This PR specifies that behavior. Moreover, it adds an example on how to listen for the 'drain' event correctly. See: nodejs@f347dad PR-URL: nodejs#10631 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
The doc specified that writable.write() was advisory only. However, ignoring that value might lead to memory leaks. This PR specifies that behavior. Moreover, it adds an example on how to listen for the 'drain' event correctly. See: nodejs@f347dad PR-URL: nodejs#10631 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
The doc specified that writable.write() was advisory only. However, ignoring that value might lead to memory leaks. This PR specifies that behavior. Moreover, it adds an example on how to listen for the 'drain' event correctly. See: f347dad PR-URL: #10631 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This did not land cleanly on v4.x. Please feel free to manually backport |
Do we really want this on v4? I'm happy to backport if it's needed. |
I've no problem with it going into v4 unless the other changes are disruptive or it's just too much work |
The doc specified that writable.write() was advisory only. However, ignoring that value might lead to memory leaks. This PR specifies that behavior. Moreover, it adds an example on how to listen for the 'drain' event correctly. See: f347dad PR-URL: #10631 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
The doc specified that writable.write() was advisory only. However, ignoring that value might lead to memory leaks. This PR specifies that behavior. Moreover, it adds an example on how to listen for the 'drain' event correctly. See: nodejs@f347dad PR-URL: nodejs#10631 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Backported to v4 in #11824. |
The doc specified that writable.write() was advisory only. However,
ignoring that value might lead to memory leaks. This PR specifies that
behavior. Moreover, it adds an example on how to listen for the 'drain'
event correctly.
Checklist
Affected core subsystem(s)
doc, stream
cc @nodejs/streams @binki @Tanuja-Sawant @silverwind
f347dad