stream: defer readable when sync #18515

mafintosh · 2018-02-01T22:25:14Z

Checklist

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

Affected core subsystem(s)

stream

Fixes #18484 by not calling flow() when state.sync is set

mafintosh · 2018-02-01T22:25:34Z

/cc @mcollina @nodejs/streams

addaleax · 2018-02-02T00:52:09Z

Hi! :)

I can’t tell whether this patch is semantically correct on its own, but while looking into #18512, I checked and my patch at #18516 actually fixes both of the issues (at least going by the test commit provided here, which I’ve included in my PR). I’d be glad if you could take a look at that?

mafintosh · 2018-02-02T09:02:10Z

@addaleax both PRs are valid. By #18512 you fix the symptom of this PR however the contract that no data will be emitted when flowing and the stream is paused is still broken. This PR fixes that.

The root cause here is that read() triggers _read which can trigger onEofChunk synchronously.
onEofChunk without this PR will trigger a flow() in the same tick causing data to be emitted under certain scenarios. However after this flow() the original read() call might trigger more data to be emitted even if the stream has been paused. This will break a lot userland code i fear :)

addaleax · 2018-02-02T10:16:56Z

Thanks for the explanation, that was helpful!

By #18512 you fix the symptom of this PR however the contract that no data will be emitted when flowing and the stream is paused is still broken. This PR fixes that.

Not a blocker or anything, but could you add a test or similar code that doesn’t use .pipe() here?

mcollina

LGTM with the additional test @addaleax required.

mafintosh · 2018-02-03T09:31:59Z

@addaleax @mcollina added a test that only uses pause/resume

mcollina · 2018-02-03T10:43:56Z

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

mafintosh · 2018-02-05T11:20:36Z

@mcollina fixed the other failing test by checking if there is any data in the buffer before nextTicking it

mcollina · 2018-02-05T11:28:02Z

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

mcollina

LGTM

mcollina · 2018-02-06T08:55:58Z

Landed as 563fff2.

PR-URL: #18515 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The complicated `awaitDrain` machinery can be made a bit slimmer, and more correct, by just resetting the value each time `stream.emit('data')` is called. By resetting the value before emitting the data chunk, and seeing whether any pipe destinations return `.write() === false`, we always end up in a consistent state and don’t need to worry about odd situations (like `dest.write(chunk)` emitting more data). PR-URL: nodejs#18516 Fixes: nodejs#18484 Fixes: nodejs#18512 Refs: nodejs#18515 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This reverts commit 563fff2 as it was causing failures in CITGM with `dicer`. Refs: nodejs#18515

Refs: nodejs#18515 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fix a regression introduced by #18515 that broke the dicer module tests. See: #18515 PR-URL: #18615 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MylesBorins · 2018-02-21T00:10:05Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

mcollina · 2018-02-21T09:20:01Z

This should not be backported.

The complicated `awaitDrain` machinery can be made a bit slimmer, and more correct, by just resetting the value each time `stream.emit('data')` is called. By resetting the value before emitting the data chunk, and seeing whether any pipe destinations return `.write() === false`, we always end up in a consistent state and don’t need to worry about odd situations (like `dest.write(chunk)` emitting more data). PR-URL: nodejs#18516 Fixes: nodejs#18484 Fixes: nodejs#18512 Refs: nodejs#18515 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#18515 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The complicated `awaitDrain` machinery can be made a bit slimmer, and more correct, by just resetting the value each time `stream.emit('data')` is called. By resetting the value before emitting the data chunk, and seeing whether any pipe destinations return `.write() === false`, we always end up in a consistent state and don’t need to worry about odd situations (like `dest.write(chunk)` emitting more data). PR-URL: nodejs#18516 Fixes: nodejs#18484 Fixes: nodejs#18512 Refs: nodejs#18515 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Fix a regression introduced by nodejs#18515 that broke the dicer module tests. See: nodejs#18515 PR-URL: nodejs#18615 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Feb 1, 2018

mafintosh changed the title ~~Defer readable when sync~~ stream: defer readable when sync Feb 1, 2018

addaleax mentioned this pull request Feb 2, 2018

stream: always reset awaitDrain when emitting data #18516

Merged

3 tasks

jasnell requested a review from mcollina February 2, 2018 16:47

mcollina approved these changes Feb 2, 2018

View reviewed changes

jasnell approved these changes Feb 3, 2018

View reviewed changes

stream: add failing pipe test

1f480f6

mafintosh force-pushed the defer-readable-when-sync branch from a8fbe3a to 620598c Compare February 3, 2018 09:31

mafintosh force-pushed the defer-readable-when-sync branch from 620598c to 1d70502 Compare February 3, 2018 09:33

stream: add failing pause -> resume -> pause test

591a1c5

mafintosh force-pushed the defer-readable-when-sync branch from 1d70502 to a83008a Compare February 5, 2018 11:19

stream: defer readable and flow when sync

a83008a

mcollina approved these changes Feb 5, 2018

View reviewed changes

mcollina closed this Feb 6, 2018

mcollina pushed a commit that referenced this pull request Feb 6, 2018

stream: defer readable and flow when sync

563fff2

PR-URL: #18515 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit to addaleax/node that referenced this pull request Feb 7, 2018

Revert "stream: defer readable and flow when sync"

203e607

This reverts commit 563fff2 as it was causing failures in CITGM with `dicer`. Refs: nodejs#18515

addaleax mentioned this pull request Feb 7, 2018

Revert "stream: defer readable and flow when sync" #18612

Closed

addaleax pushed a commit to addaleax/node that referenced this pull request Feb 7, 2018

test: keep test/parallel/test-stream-pipe-flow

1779c7d

Refs: nodejs#18515 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added the backport-requested-v9.x label Feb 21, 2018

mcollina added dont-land-on-v4.x and removed backport-requested-v9.x labels Feb 21, 2018

mcollina mentioned this pull request Mar 7, 2018

[v9.x] stream: always reset awaitDrain when emitting data #18911

Closed

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

stream: defer readable and flow when sync

e0c9d43

PR-URL: nodejs#18515 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: defer readable when sync #18515

stream: defer readable when sync #18515

mafintosh commented Feb 1, 2018

mafintosh commented Feb 1, 2018

addaleax commented Feb 2, 2018

mafintosh commented Feb 2, 2018

addaleax commented Feb 2, 2018

mcollina left a comment

mafintosh commented Feb 3, 2018 •

edited

mcollina commented Feb 3, 2018

mafintosh commented Feb 5, 2018

mcollina commented Feb 5, 2018

mcollina left a comment

mcollina commented Feb 6, 2018

MylesBorins commented Feb 21, 2018

mcollina commented Feb 21, 2018

stream: defer readable when sync #18515

stream: defer readable when sync #18515

Conversation

mafintosh commented Feb 1, 2018

Checklist

Affected core subsystem(s)

mafintosh commented Feb 1, 2018

addaleax commented Feb 2, 2018

mafintosh commented Feb 2, 2018

addaleax commented Feb 2, 2018

mcollina left a comment

Choose a reason for hiding this comment

mafintosh commented Feb 3, 2018 • edited

mcollina commented Feb 3, 2018

mafintosh commented Feb 5, 2018

mcollina commented Feb 5, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Feb 6, 2018

MylesBorins commented Feb 21, 2018

mcollina commented Feb 21, 2018

mafintosh commented Feb 3, 2018 •

edited