doc: Unclear relationship between `_read` and `stream.push` return value

[sploders101]

📗 API Reference Docs Problem

• Version: v15.12.0

• Platform: Darwin Shauns-MacBook-Pro.local 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:07:06 PST 2021; root:xnu-7195.81.3~1/RELEASE_X86_64 x86_64

• Subsystem: stream

Location

Section of the site where the content exists

Affected URL(s):

• https://nodejs.org/api/stream.html#stream_readable_read_size_1

Description

Concise explanation of the problem

The documentation for implementing a readable stream indicates that the _read function should continue reading from the resource until stream.push returns false. When I implemented my stream this way, I saw the opposite behavior. stream._read was called every time it ran stream.push(...), resulting in hundreds of concurrent read operations.

More Detail

When readable._read() is called, if data is available from the resource, the implementation should begin pushing that data into the read queue using the this.push(dataChunk) method. _read() should continue reading from the resource and pushing data until readable.push() returns false. Only when _read() is called again after it has stopped should it resume pushing additional data onto the queue.

I quickly noticed a memory leak in my application, because readable._read was getting called every time I ran readable.push(...) (when the buffer was below highWaterMark), which meant there may be hundreds of concurrent read operations on the same resource. I would like to reword this section to eliminate this ambiguity and clarify that the _read function may be called multiple times before reaching the highWaterMark.

I would be interested in contributing to this, but I would want some clarification first. Why would we need to listen to the result of stream.push(...) if _read doesn't get called when the buffer is full? For edge cases where data is pushed outside the _read function, stream.push(...)'s return value is documented here, but it doesn't seem to have a use from within _read.

---

• I would like to work on this issue and
  submit a pull request.

[mscdex]

You should pay attention to .push()'s return value no matter where you call it because you don't know when the internal buffer will fill up. Even the size argument passed to _read() is labeled as "advisory" (which is not documented as being the exact number of bytes left to fill the internal buffer) and you can also push more data than that value anyway (especially where it's more convenient/efficient to grab in larger chunks from upstream).

Paying attention to the return value is most important if it's possible for .push() to be called more than once in your _read() handler because if .push() returned true after the first call, there's no point in calling it again because then you're just unnecessarily increasing memory usage of the stream at that point.

[sploders101]

That makes a little more sense. How would one utilize the return value of stream.push(...) if they only call it once from the _read function? The stream shouldn't be trying to read more data if the buffer is full, right? If it is, how can we tell when it's okay to read more data in without pushing?

[mscdex]

The stream shouldn't be trying to read more data if the buffer is full, right?

Without checking, I believe this is the case, yes.

[sploders101]

Ok, I think I know enough to start working on a pull request then, since someone will have to check it before it makes it into production, regardless. I've never updated node's documentation before though. Is it just the .md files in the docs folder?

[aduh95]

Is it just the .md files in the docs folder?

Yes that's correct. You can use make test-doc -j to run the linter and tests on your local machine, and make docserve -j to preview the HTML rendered version.