stream: do not unconditionally call `_read()` on `resume()` by addaleax · Pull Request #26965 · nodejs/node

addaleax · 2019-03-28T20:17:19Z

readable.resume() calls .read(0), which in turn previously set
needReadable = true, and so a subsequent .read() call would
call _read() even though enough data was already available.

This can lead to elevated memory usage, because calling _read()
when enough data is in the readable buffer means that backpressure
is not being honoured.

Fixes: #26957

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

`readable.resume()` calls `.read(0)`, which in turn previously set `needReadable = true`, and so a subsequent `.read()` call would call `_read()` even though enough data was already available. This can lead to elevated memory usage, because calling `_read()` when enough data is in the readable buffer means that backpressure is not being honoured. Fixes: nodejs#26957

nodejs-github-bot · 2019-03-28T20:17:22Z

@addaleax Sadly, an error occurred when I tried to trigger a build. :(

addaleax · 2019-03-28T20:18:25Z

/cc @nodejs/streams PTAL – does this look like the right approach to you? I’m particularly unsure if this bit makes sense in the first place:

node/lib/_stream_readable.js

Lines 440 to 441 in 2c73868

    
           // if we need a readable event, then we need to do some reading. 
        
           var doRead = state.needReadable;

mcollina

LGTM

mcollina · 2019-03-28T20:20:24Z

This makes sense to me. However this is so complex that we might found a bug in 6 months ;).

nodejs-github-bot · 2019-03-28T20:21:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/21992/

zbjornson · 2019-03-28T22:04:20Z

🙌 this fixes the buffering part of #25057. (I believe it's still technically possible for read() to bite off more from the internal buffer than can be concatenated, but now that buffer shouldn't be >kMaxLength.)

ronag · 2019-03-31T12:02:06Z

May I suggest we fast-track this?

addaleax · 2019-03-31T12:15:23Z

@ronag This is ready to be merged anyway – I’d be careful about backporting a change like this to LTS faster than usual. I understand that this is an important fix, but at the same time we’ve run into issues with streams changes breaking people’s code much more often than we’d like. /cc @nodejs/lts

Landed in 20c3ac2

`readable.resume()` calls `.read(0)`, which in turn previously set `needReadable = true`, and so a subsequent `.read()` call would call `_read()` even though enough data was already available. This can lead to elevated memory usage, because calling `_read()` when enough data is in the readable buffer means that backpressure is not being honoured. Fixes: #26957 PR-URL: #26965 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

`readable.resume()` calls `.read(0)`, which in turn previously set `needReadable = true`, and so a subsequent `.read()` call would call `_read()` even though enough data was already available. This can lead to elevated memory usage, because calling `_read()` when enough data is in the readable buffer means that backpressure is not being honoured. Fixes: #26957 PR-URL: #26965 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>

mcollina · 2019-06-03T09:48:40Z

@nodejs/releasers would you mind bacporting this in the next Node 10 release? thanks!

ronag · 2019-07-03T08:49:16Z

@mcollina any news here? This is rather serious for us and we have no LTS to fallback to...

mcollina · 2019-07-03T09:30:00Z

@ronag what's the question? Are you asking for this to be included in the next version of Node v10?

mcollina · 2019-07-03T09:30:47Z

@nodejs/lts would you mind backporting this?

`readable.resume()` calls `.read(0)`, which in turn previously set `needReadable = true`, and so a subsequent `.read()` call would call `_read()` even though enough data was already available. This can lead to elevated memory usage, because calling `_read()` when enough data is in the readable buffer means that backpressure is not being honoured. Fixes: #26957 PR-URL: #26965 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable changes: - **stream**: do not unconditionally call `\_read()` on `resume()` (Anna Henningsen) [#26965](#26965) - **worker**: fix nullptr deref after MessagePort deser failure (Anna Henningsen) [#25076](#25076)

Notable changes: - **deps**: upgrade openssl sources to 1.1.1c (Sam Roberts) [#28212](#28212) - **stream**: do not unconditionally call `\_read()` on `resume()` (Anna Henningsen) [#26965](#26965) - **worker**: fix nullptr deref after MessagePort deser failure (Anna Henningsen) [#25076](#25076) PR-URL: #28731

addaleax added stream Issues and PRs related to the stream subsystem. lts-watch-v6.x labels Mar 28, 2019

mcollina approved these changes Mar 28, 2019

View reviewed changes

lpinca approved these changes Mar 28, 2019

View reviewed changes

addaleax mentioned this pull request Mar 28, 2019

http: Memory Leak #26957

Closed

BridgeAR approved these changes Mar 28, 2019

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 28, 2019

addaleax mentioned this pull request Mar 28, 2019

Unexpected RangeError [ERR_INVALID_OPT_VALUE] while reading from a HTTP download stream #25057

Closed

addaleax closed this Mar 31, 2019

addaleax deleted the resume-read0 branch March 31, 2019 12:15

BethGriggs mentioned this pull request Apr 9, 2019

v11.14.0 proposal #27163

Merged

BethGriggs added land-on-v10.x and removed lts-watch-v10.x labels Jul 9, 2019

BethGriggs mentioned this pull request Jul 17, 2019

v10.16.1 proposal #28731

Merged

Hakerh400 mentioned this pull request Jul 26, 2019

httpresponse pause()/resume() leads to out-of-memory error when using local server nodejs/help#2074

Closed

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

zamnuts mentioned this pull request Mar 3, 2020

Node 8 deprecation party 🎉 googleapis/google-cloud-node#2972

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

stream: do not unconditionally call `_read()` on `resume()`#26965

stream: do not unconditionally call `_read()` on `resume()`#26965
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:resume-read0

addaleax commented Mar 28, 2019

Uh oh!

nodejs-github-bot commented Mar 28, 2019

Uh oh!

addaleax commented Mar 28, 2019

Uh oh!

mcollina left a comment

Uh oh!

mcollina commented Mar 28, 2019

Uh oh!

nodejs-github-bot commented Mar 28, 2019

Uh oh!

zbjornson commented Mar 28, 2019

Uh oh!

ronag commented Mar 31, 2019

Uh oh!

addaleax commented Mar 31, 2019

Uh oh!

mcollina commented Jun 3, 2019

Uh oh!

ronag commented Jul 3, 2019

Uh oh!

mcollina commented Jul 3, 2019

Uh oh!

mcollina commented Jul 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

addaleax commented Mar 28, 2019

Checklist

Uh oh!

nodejs-github-bot commented Mar 28, 2019

Uh oh!

addaleax commented Mar 28, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Mar 28, 2019

Uh oh!

nodejs-github-bot commented Mar 28, 2019

Uh oh!

zbjornson commented Mar 28, 2019

Uh oh!

ronag commented Mar 31, 2019

Uh oh!

addaleax commented Mar 31, 2019

Uh oh!

mcollina commented Jun 3, 2019

Uh oh!

ronag commented Jul 3, 2019

Uh oh!

mcollina commented Jul 3, 2019

Uh oh!

mcollina commented Jul 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants