stream: reset awaitDrain after manual .resume() by addaleax · Pull Request #7160 · nodejs/node

addaleax · 2016-06-05T11:48:29Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

stream

Description of change

Reset the readableState.awaitDrain counter after manual calls to .resume().

What might happen otherwise is that a slow consumer at the end of the pipe could end up stalling the piping in the following scenario:

The writable stream indicates that its buffer is full.
This leads the readable stream to pause() and increase its awaitDrain counter, which will be decreased by the writable’s next drain event.
Something calls .resume() manually.
The readable continues to pipe to the writable, but once again the writable stream indicates that the buffer is full.
The awaitDrain counter is thus increased again, but since it has now been increased twice for a single piping destination, the next drain event will not be able to reset awaitDrain to zero.
The pipe is stalled and no data is passed along anymore.

The solution in this commit is to reset the awaitDrain counter to zero when resume() is called.

Fixes: #7159

This bug is has been around at least since 0.12, and I don’t currently know why it never popped up before #2325 when the awaitDrain mechanism worked like it does today.

/cc @nodejs/streams

Reset the `readableState.awaitDrain` counter after manual calls to `.resume()`. What might happen otherwise is that a slow consumer at the end of the pipe could end up stalling the piping in the following scenario: 1. The writable stream indicates that its buffer is full. 2. This leads the readable stream to `pause()` and increase its `awaitDrain` counter, which will be decreased by the writable’s next `drain` event. 3. Something calls `.resume()` manually. 4. The readable continues to pipe to the writable, but once again the writable stream indicates that the buffer is full. 5. The `awaitDrain` counter is thus increased again, but since it has now been increased twice for a single piping destination, the next `drain` event will not be able to reset `awaitDrain` to zero. 6. The pipe is stalled and no data is passed along anymore. The solution in this commit is to reset the `awaitDrain` counter to zero when `resume()` is called. Fixes: nodejs#7159

addaleax · 2016-06-05T11:50:19Z

CI: https://ci.nodejs.org/job/node-test-commit/3655/

mcollina · 2016-06-06T06:50:46Z

I think this should be ok to backport, but we should probably check CIGTM before that. cc @calvinmetcalf

LGTM

jasnell · 2016-06-06T14:48:27Z

LGTM if @nodejs/streams folks are happy.

addaleax · 2016-06-13T15:06:00Z

@thealphanerd since you self-assigned this… anything in particular you’d like to see before landing?

calvinmetcalf · 2016-06-13T16:30:18Z

Doesn't look to be anything to controversial from my perspective

MylesBorins · 2016-06-13T18:46:44Z

@calvinmetcalf I'm assuming you did not mean to close this 😄

MylesBorins · 2016-06-13T18:47:19Z

@addaleax Nothing I need to see before landing, assigned to myself to keep an eye on it for LTS

calvinmetcalf · 2016-06-13T18:56:24Z

@thealphanerd you are correct

sorry foks

mafintosh · 2016-06-13T19:11:39Z

as long as @mcollina is happy i'm happy so 👍

Reset the `readableState.awaitDrain` counter after manual calls to `.resume()`. What might happen otherwise is that a slow consumer at the end of the pipe could end up stalling the piping in the following scenario: 1. The writable stream indicates that its buffer is full. 2. This leads the readable stream to `pause()` and increase its `awaitDrain` counter, which will be decreased by the writable’s next `drain` event. 3. Something calls `.resume()` manually. 4. The readable continues to pipe to the writable, but once again the writable stream indicates that the buffer is full. 5. The `awaitDrain` counter is thus increased again, but since it has now been increased twice for a single piping destination, the next `drain` event will not be able to reset `awaitDrain` to zero. 6. The pipe is stalled and no data is passed along anymore. The solution in this commit is to reset the `awaitDrain` counter to zero when `resume()` is called. Fixes: #7159 PR-URL: #7160 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2016-06-13T21:43:38Z

Landed in e2e615e, thanks for the reviews and comments, everyone!

Reset the `readableState.awaitDrain` counter after manual calls to `.resume()`. What might happen otherwise is that a slow consumer at the end of the pipe could end up stalling the piping in the following scenario: 1. The writable stream indicates that its buffer is full. 2. This leads the readable stream to `pause()` and increase its `awaitDrain` counter, which will be decreased by the writable’s next `drain` event. 3. Something calls `.resume()` manually. 4. The readable continues to pipe to the writable, but once again the writable stream indicates that the buffer is full. 5. The `awaitDrain` counter is thus increased again, but since it has now been increased twice for a single piping destination, the next `drain` event will not be able to reset `awaitDrain` to zero. 6. The pipe is stalled and no data is passed along anymore. The solution in this commit is to reset the `awaitDrain` counter to zero when `resume()` is called. Fixes: #7159 PR-URL: #7160 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

fixes streams error nodejs/node#7160

Reset the `readableState.awaitDrain` counter after manual calls to `.resume()`. What might happen otherwise is that a slow consumer at the end of the pipe could end up stalling the piping in the following scenario: 1. The writable stream indicates that its buffer is full. 2. This leads the readable stream to `pause()` and increase its `awaitDrain` counter, which will be decreased by the writable’s next `drain` event. 3. Something calls `.resume()` manually. 4. The readable continues to pipe to the writable, but once again the writable stream indicates that the buffer is full. 5. The `awaitDrain` counter is thus increased again, but since it has now been increased twice for a single piping destination, the next `drain` event will not be able to reset `awaitDrain` to zero. 6. The pipe is stalled and no data is passed along anymore. The solution in this commit is to reset the `awaitDrain` counter to zero when `resume()` is called. Fixes: #7159 PR-URL: #7160 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This LTS release comes with 89 commits. This includes 46 commits that are docs related, 11 commits that are test related, 8 commits that are build related, and 4 commits that are benchmark related. Notable Changes: - debugger: - All properties of an array (aside from length) can now be printed in the repl (cjihrig) #6448 - npm: - Upgrade npm to 2.15.8 (Rebecca Turner) #7412 - stream: - Fix for a bug that became more prevalent with the stream changes that landed in v4.4.5. (Anna Henningsen) #7160 - V8: - Fix for a bug in crankshaft that was causing crashes on arm64 (Myles Borins) #7442 - Add missing classes to postmortem info such as JSMap and JSSet (evan.lucas) #3792

johnaAr555

var state = this._readableState;
if (!state.flowing) {
debug('resume');

johnaAr555

var state = this._readableState;
if (!state.flowing) {
debug('resume');

addaleax added the stream Issues and PRs related to the stream subsystem. label Jun 5, 2016

addaleax mentioned this pull request Jun 5, 2016

'close' event for spawn() isn't emitted under unknown conditions #7159

Closed

addaleax added the lts-watch-v4.x label Jun 5, 2016

addaleax mentioned this pull request Jun 7, 2016

child-process: data loss with piped stdout #7184

Closed

petrosagg mentioned this pull request Jun 7, 2016

Fix child process stdio data loss with slow piped consumers #7185

Closed

4 tasks

MylesBorins self-assigned this Jun 8, 2016

MylesBorins mentioned this pull request Jun 8, 2016

Regression in v4.4.5 nodejs/Release#110

Closed

petrosagg mentioned this pull request Jun 10, 2016

Add two child-process stdio flush tests #7257

Closed

3 tasks

addaleax mentioned this pull request Jun 12, 2016

Change in stream behaviour in Node 4.4.5 #7278

Closed

calvinmetcalf closed this Jun 13, 2016

MylesBorins reopened this Jun 13, 2016

MylesBorins mentioned this pull request Jun 13, 2016

Streams Regression in v4.x nodejs/Release#112

Closed

addaleax closed this Jun 13, 2016

addaleax deleted the fix-7159 branch June 13, 2016 21:58

evanlucas mentioned this pull request Jun 16, 2016

v6.2.2 proposal #7323

Merged

abresas added a commit to balena-io-modules/open-balena-base that referenced this pull request Jun 20, 2016

move to nodejs v6.2.2

7194190

fixes streams error nodejs/node#7160

abresas added a commit to balena-io-modules/open-balena-base that referenced this pull request Jun 20, 2016

move to nodejs v6.2.2

4e06ca1

fixes streams error nodejs/node#7160

abresas mentioned this pull request Jun 20, 2016

move to nodejs v6.2.2 balena-io-modules/open-balena-base#15

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jun 23, 2016

MylesBorins mentioned this pull request Jun 24, 2016

v4.4.7 proposal #7395

Merged

MylesBorins removed their assignment Dec 27, 2016

johnaAr555 reviewed Apr 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

stream: reset awaitDrain after manual .resume()#7160

stream: reset awaitDrain after manual .resume()#7160
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:fix-7159

addaleax commented Jun 5, 2016

Uh oh!

addaleax commented Jun 5, 2016

Uh oh!

mcollina commented Jun 6, 2016

Uh oh!

jasnell commented Jun 6, 2016

Uh oh!

addaleax commented Jun 13, 2016

Uh oh!

calvinmetcalf commented Jun 13, 2016

Uh oh!

MylesBorins commented Jun 13, 2016

Uh oh!

MylesBorins commented Jun 13, 2016

Uh oh!

calvinmetcalf commented Jun 13, 2016

Uh oh!

mafintosh commented Jun 13, 2016

Uh oh!

addaleax commented Jun 13, 2016

Uh oh!

johnaAr555 left a comment

Uh oh!

johnaAr555 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

addaleax commented Jun 5, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

addaleax commented Jun 5, 2016

Uh oh!

mcollina commented Jun 6, 2016

Uh oh!

jasnell commented Jun 6, 2016

Uh oh!

addaleax commented Jun 13, 2016

Uh oh!

calvinmetcalf commented Jun 13, 2016

Uh oh!

MylesBorins commented Jun 13, 2016

Uh oh!

MylesBorins commented Jun 13, 2016

Uh oh!

calvinmetcalf commented Jun 13, 2016

Uh oh!

mafintosh commented Jun 13, 2016

Uh oh!

addaleax commented Jun 13, 2016

Uh oh!

johnaAr555 left a comment

Choose a reason for hiding this comment

Uh oh!

johnaAr555 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants