stream: reset flowing state if no 'readable' or 'data' listeners. #31036

ronag · 2019-12-20T14:59:14Z

If we don't have any 'readable' or 'data' listeners and we are not about to resume. Then reset flowing state to initial null state.

Fixes: #24474

I don't feel very confident modifying things here but this seems to solve the issue and doesn't break any existing tests.

I would very much like a CITGM on this.

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

If we don't have any 'readable' or 'data' listeners and we are not about to resume. Then reset flowing state to initial null state. Fixes: nodejs#24474

nodejs-github-bot · 2019-12-20T15:51:03Z

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

BridgeAR · 2019-12-20T15:51:22Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2150/ (queued)

lpinca · 2019-12-21T06:41:06Z

I'm not sure if resetting to null has side effects but if it does not break anything this LGTM.

lpinca · 2019-12-21T06:41:18Z

@nodejs/streams

lpinca · 2019-12-21T07:20:28Z

A possible side effect is

'use strict';

const assert = require('assert');
const { Readable } = require('stream');

const readable = new Readable({
  read() {}
});

function read() {}

readable.setEncoding('utf8');
readable.on('readable', read);
readable.removeListener('readable', read);
readable.pause();

process.nextTick(function() {
  assert(readable.isPaused()); // Throws.
});

ronag · 2019-12-21T07:46:04Z

A possible side effect is

Good point! I've tried fixing this as well. However, now I've slightly modified the initial state of paused which might break things outside of test. I could put it behind a private symbol and create a getter to emulate the previous behavior?

Could someone add the WIP label on this so this doesn't accidentally land until the above comment has either been ok:d or resolved?

lib/_stream_readable.js

ronag · 2019-12-21T07:51:00Z

Tried to sort the compat issue.

lpinca · 2019-12-21T08:22:12Z

There is one legitimate test failure because readable.isPaused() no longer uses state.flowing.

node/test/parallel/test-tls-server-parent-constructor-options.js

Line 49 in 118b28a

assert.strictEqual(socket.isPaused(), true);

I think as is this is semver-major.

ronag · 2019-12-21T09:37:07Z

Tried fixing that as well.

nodejs-github-bot · 2019-12-21T10:30:12Z

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

mcollina

lgtm as semver-major

ronag · 2019-12-21T11:41:12Z

@Trott: Can we get a CITGM?

lpinca · 2019-12-21T11:42:00Z

I think it doesn't necessarily have to be semver-major now. Could be patch.

lpinca · 2019-12-21T11:45:29Z

~~CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2151/~~
~~CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2152/~~
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2156/ ✅ (only known flakes. moment seems to have regressed recently)

nodejs-github-bot · 2019-12-21T12:16:53Z

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

nodejs-github-bot · 2019-12-24T16:44:21Z

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

nodejs-github-bot · 2019-12-25T10:29:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27935/ ✅ (one known flake on arm)

BridgeAR · 2019-12-25T10:30:00Z

@mcollina are you fine with this being a patch as suggested by @lpinca?

mcollina · 2019-12-25T10:45:43Z

yes, however I would tag this as “baking for lts.

If we don't have any 'readable' or 'data' listeners and we are not about to resume. Then reset flowing state to initial null state. PR-URL: #31036 Fixes: #24474 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

BridgeAR · 2019-12-25T11:14:30Z

Landed in 3d47c85 🎉

If we don't have any 'readable' or 'data' listeners and we are not about to resume. Then reset flowing state to initial null state. PR-URL: #31036 Fixes: #24474 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Dec 20, 2019

stream: reset flowing state if no 'readable' or 'data' listeners.

e70f3f3

If we don't have any 'readable' or 'data' listeners and we are not about to resume. Then reset flowing state to initial null state. Fixes: nodejs#24474

ronag force-pushed the readable-data branch from 38e4b06 to e70f3f3 Compare December 20, 2019 15:04

fixup: add test

1514fb9

lpinca approved these changes Dec 21, 2019

View reviewed changes

fixup: pasued

2a27826

ronag commented Dec 21, 2019

View reviewed changes

lib/_stream_readable.js Outdated Show resolved Hide resolved

lpinca added the wip Issues and PRs that are still a work in progress. label Dec 21, 2019

ronag requested a review from lpinca December 21, 2019 07:51

fixup: compat paused

906fd31

ronag force-pushed the readable-data branch from f8c08a8 to 906fd31 Compare December 21, 2019 07:51

lpinca approved these changes Dec 21, 2019

View reviewed changes

lpinca removed the wip Issues and PRs that are still a work in progress. label Dec 21, 2019

fixup: paused

a080fd1

ronag force-pushed the readable-data branch from 3cae341 to a080fd1 Compare December 21, 2019 11:00

mcollina approved these changes Dec 21, 2019

View reviewed changes

Trott approved these changes Dec 21, 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 Dec 25, 2019

BridgeAR added the baking-for-lts PRs that need to wait before landing in a LTS release. label Dec 25, 2019

BridgeAR closed this Dec 25, 2019

BridgeAR mentioned this pull request Jan 7, 2020

v13.6.0 proposal #31238

Merged

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: reset flowing state if no 'readable' or 'data' listeners. #31036

stream: reset flowing state if no 'readable' or 'data' listeners. #31036

ronag commented Dec 20, 2019 •

edited

Loading

nodejs-github-bot commented Dec 20, 2019

BridgeAR commented Dec 20, 2019

lpinca commented Dec 21, 2019

lpinca commented Dec 21, 2019

lpinca commented Dec 21, 2019 •

edited

Loading

ronag commented Dec 21, 2019 •

edited

Loading

ronag commented Dec 21, 2019

lpinca commented Dec 21, 2019 •

edited

Loading

ronag commented Dec 21, 2019

nodejs-github-bot commented Dec 21, 2019

mcollina left a comment

ronag commented Dec 21, 2019

lpinca commented Dec 21, 2019 •

edited

Loading

lpinca commented Dec 21, 2019 •

edited by BridgeAR

Loading

nodejs-github-bot commented Dec 21, 2019

nodejs-github-bot commented Dec 24, 2019

nodejs-github-bot commented Dec 25, 2019 •

edited by BridgeAR

Loading

BridgeAR commented Dec 25, 2019

mcollina commented Dec 25, 2019

BridgeAR commented Dec 25, 2019

stream: reset flowing state if no 'readable' or 'data' listeners. #31036

stream: reset flowing state if no 'readable' or 'data' listeners. #31036

Conversation

ronag commented Dec 20, 2019 • edited Loading

Checklist

nodejs-github-bot commented Dec 20, 2019

BridgeAR commented Dec 20, 2019

lpinca commented Dec 21, 2019

lpinca commented Dec 21, 2019

lpinca commented Dec 21, 2019 • edited Loading

ronag commented Dec 21, 2019 • edited Loading

ronag commented Dec 21, 2019

lpinca commented Dec 21, 2019 • edited Loading

ronag commented Dec 21, 2019

nodejs-github-bot commented Dec 21, 2019

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Dec 21, 2019

lpinca commented Dec 21, 2019 • edited Loading

lpinca commented Dec 21, 2019 • edited by BridgeAR Loading

nodejs-github-bot commented Dec 21, 2019

nodejs-github-bot commented Dec 24, 2019

nodejs-github-bot commented Dec 25, 2019 • edited by BridgeAR Loading

BridgeAR commented Dec 25, 2019

mcollina commented Dec 25, 2019

BridgeAR commented Dec 25, 2019

ronag commented Dec 20, 2019 •

edited

Loading

lpinca commented Dec 21, 2019 •

edited

Loading

ronag commented Dec 21, 2019 •

edited

Loading

lpinca commented Dec 21, 2019 •

edited

Loading

lpinca commented Dec 21, 2019 •

edited

Loading

lpinca commented Dec 21, 2019 •

edited by BridgeAR

Loading

nodejs-github-bot commented Dec 25, 2019 •

edited by BridgeAR

Loading