Clarify when readable._read(...) is called. - #38726
Conversation
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
Sorry, something went wrong.
| called again after it has stopped should it resume pushing additional data into | ||
| the queue. |
There was a problem hiding this comment.
Was this change onto the queue → into the queue intentional? I noticed that into the read queue was used above, on line 2512, even before this change.
Sorry, something went wrong.
There was a problem hiding this comment.
It was not intentional, but I think accomplishes the same goal. I tried to reword the existing paragraph, but it always seemed confusing and contradictory, so I ended up just rewriting it. I think into makes more sense though. I'd only use onto if I were saying I'm pushing onto *the end of* the queue.
Sorry, something went wrong.
Fixes: #38586
This PR clarifies some confusion with the readable._read(...) function dealing with when it is called. The current version of the documentation seems to suggest that _read() is responsible for looping itself to continually request data until this.push(...) returns false, but this is not the case. After implementing a readable stream this way, I quickly found memory leaks in my code, because Node.JS was calling my _read function after every this.push(...), but I was also looping inside _read, resulting in an exponentially increasing number of reads on the remote resource. Hopefully this will help to clarify this behavior.
This is my first PR against NodeJS, so please let me know if there's anything I could do better.