Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
addaleax wants to merge 1 commit into from
Closed

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

addaleax wants to merge 1 commit into from

Conversation

@addaleax
Copy link
Member

@addaleaxaddaleax commented Mar 28, 2019

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

@addaleax
stream: do not unconditionally call _read() on resume()
5c72c6d
`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
@addaleaxaddaleax added stream Issues and PRs related to the stream subsystem. lts-watch-v6.x labels Mar 28, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 28, 2019

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

@addaleax
Copy link
MemberAuthor

addaleax commented Mar 28, 2019

/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.
vardoRead=state.needReadable;

mcollina
mcollina approved these changes Mar 28, 2019
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

mcollina commented Mar 28, 2019

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 28, 2019

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

@addaleaxaddaleax mentioned this pull request Mar 28, 2019
Closed
@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 28, 2019
@zbjornson
Copy link
Contributor

zbjornson commented Mar 28, 2019

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

@addaleaxaddaleax mentioned this pull request Mar 28, 2019
Closed
@ronag
Copy link
Member

ronag commented Mar 31, 2019

May I suggest we fast-track this?

@addaleax
Copy link
MemberAuthor

addaleax commented Mar 31, 2019

@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

@addaleaxaddaleax deleted the resume-read0 branch March 31, 2019 12:15
addaleax added a commit that referenced this pull request Mar 31, 2019
@addaleax
stream: do not unconditionally call _read() on resume()
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
@addaleax@BethGriggs
stream: do not unconditionally call _read() on resume()
b5d1680
`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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
@addaleax@BethGriggs
stream: do not unconditionally call _read() on resume()
732d376
`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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Signed-off-by: Beth Griggs <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
@addaleax@BethGriggs
stream: do not unconditionally call _read() on resume()
ea47189
`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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Signed-off-by: Beth Griggs <[email protected]>
@BethGriggsBethGriggs mentioned this pull request Apr 9, 2019
Merged
@mcollina
Copy link
Member

mcollina commented Jun 3, 2019

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

@ronag
Copy link
Member

ronag commented Jul 3, 2019

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

@mcollina
Copy link
Member

mcollina commented Jul 3, 2019

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

@mcollina
Copy link
Member

mcollina commented Jul 3, 2019

@nodejs/lts would you mind backporting this?

BethGriggs pushed a commit that referenced this pull request Jul 9, 2019
@addaleax@BethGriggs
stream: do not unconditionally call _read() on resume()
7f3c29c
`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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jul 17, 2019
@addaleax@BethGriggs
stream: do not unconditionally call _read() on resume()
274b97c
`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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs added a commit that referenced this pull request Jul 17, 2019
@BethGriggs
2019-05-28, Version 10.16.1 'Dubnium' (LTS)
4d0e87c
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)
@BethGriggsBethGriggs mentioned this pull request Jul 17, 2019
Merged
@Hakerh400Hakerh400 mentioned this pull request Jul 26, 2019
Closed
BethGriggs added a commit that referenced this pull request Jul 26, 2019
@BethGriggs
2019-07-30, Version 10.16.1 'Dubnium' (LTS)
2d18f84
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
BethGriggs added a commit that referenced this pull request Jul 30, 2019
@BethGriggs
2019-07-30, Version 10.16.1 'Dubnium' (LTS)
fe32fe8
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
BethGriggs added a commit that referenced this pull request Jul 31, 2019
@BethGriggs
2019-07-31, Version 10.16.1 'Dubnium' (LTS)
a395299
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
BethGriggs added a commit that referenced this pull request Jul 31, 2019
@BethGriggs
2019-07-31, Version 10.16.1 'Dubnium' (LTS)
9d37013
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
@zamnutszamnuts mentioned this pull request Mar 3, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@lpincalpincalpinca approved these changes

@BridgeARBridgeARBridgeAR approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

http: Memory Leak

8 participants

@addaleax@nodejs-github-bot@mcollina@zbjornson@ronag@lpinca@BridgeAR@BethGriggs