Skip to content
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.

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

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

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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
@addaleax addaleax added stream Issues and PRs related to the stream subsystem. lts-watch-v6.x labels Mar 28, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@addaleax
Copy link
Member Author

/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:

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

Copy link
Member

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

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

@addaleax addaleax mentioned this pull request Mar 28, 2019
@BridgeAR 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
@zbjornson
Copy link
Contributor

🙌 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
Copy link
Member

ronag commented Mar 31, 2019

May I suggest we fast-track this?

@addaleax
Copy link
Member Author

@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

@addaleax addaleax closed this Mar 31, 2019
@addaleax addaleax deleted the resume-read0 branch March 31, 2019 12:15
addaleax added a commit that referenced this pull request Mar 31, 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

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>
BethGriggs pushed a commit that referenced this pull request Apr 5, 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

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>
BethGriggs pushed a commit that referenced this pull request Apr 9, 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

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>
BethGriggs pushed a commit that referenced this pull request Apr 9, 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

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>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
@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
`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>
BethGriggs pushed a commit that referenced this pull request Jul 17, 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

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>
BethGriggs added a commit that referenced this pull request Jul 17, 2019
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)
@BethGriggs BethGriggs mentioned this pull request Jul 17, 2019
BethGriggs added a commit that referenced this pull request Jul 26, 2019
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
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
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http: Memory Leak
8 participants