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]()?

should this be inherits?

Also, same question about the linking of [Writable]() here

should we encourage consistency of async vs sync calling of the callback? Maybe wrap in setImmediate()?

I'll wrap in a process.nextTick(), as there is no need to slow things down doing a full I/O cycle.

fair enough :]

the drain link has an extra backtick at the end which will break the link.

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.

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?

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).

@binki under your definition, a Transform stream that is not piped is a memory leak, which is the point I am trying to make.

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.

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

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"?

Much better!

However, I think it would be clearer if you replace listening to `'drain'` with waiting for `'drain'` .

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

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.

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.

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.

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

@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

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 thiswrite() 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.

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?

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

I've added an example and a comment.

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][], ..."

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.

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.

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.

Capitalize sentence, end it with a period, and wrap to 80 columns.

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?

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.

I added the example because it was asked for in a review. I am happy in removing it.