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: reset flowing state if no 'readable' or 'data' listeners. #31036

Closed
wants to merge 5 commits into from

Conversation

@ronag
Copy link
Member

ronag commented Dec 20, 2019

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: #24474
@ronag ronag force-pushed the nxtedition:readable-data branch from 38e4b06 to e70f3f3 Dec 20, 2019
@nodejs-github-bot

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 20, 2019

@lpinca
lpinca approved these changes Dec 21, 2019
@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Dec 21, 2019

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

@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Dec 21, 2019

@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Dec 21, 2019

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

This comment has been minimized.

Copy link
Member Author

ronag commented Dec 21, 2019

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 Outdated Show resolved Hide resolved
@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Dec 21, 2019

Tried to sort the compat issue.

@ronag ronag requested a review from lpinca Dec 21, 2019
@ronag ronag force-pushed the nxtedition:readable-data branch from f8c08a8 to 906fd31 Dec 21, 2019
@lpinca
lpinca approved these changes Dec 21, 2019
@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Dec 21, 2019

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

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

I think as is this is semver-major.

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Dec 21, 2019

Tried fixing that as well.

@nodejs-github-bot

This comment has been minimized.

@ronag ronag force-pushed the nxtedition:readable-data branch from 3cae341 to a080fd1 Dec 21, 2019
Copy link
Member

mcollina left a comment

lgtm as semver-major

@ronag

This comment has been minimized.

Copy link
Member Author

ronag commented Dec 21, 2019

@Trott: Can we get a CITGM?

@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Dec 21, 2019

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

@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Dec 21, 2019

@Trott
Trott approved these changes Dec 21, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Dec 25, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 25, 2019

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

@mcollina

This comment has been minimized.

Copy link
Member

mcollina commented Dec 25, 2019

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

BridgeAR added a commit that referenced this pull request Dec 25, 2019
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

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 25, 2019

Landed in 3d47c85 🎉

@BridgeAR BridgeAR closed this Dec 25, 2019
BridgeAR added a commit that referenced this pull request Jan 3, 2020
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 BridgeAR mentioned this pull request Jan 7, 2020
targos added a commit that referenced this pull request Jan 14, 2020
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>
targos added a commit that referenced this pull request Jan 14, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.