stream: fix Readable stream state properties by lundibundi · Pull Request #34886 · nodejs/node

lundibundi · 2020-08-23T09:24:58Z

Looks like they have been accidentally moved in
#31144. This also adds the proxy
properties to Readable since they have been present all this time
and removing them would be breaking.

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

/cc @nodejs/streams @antsmartian

nodejs-github-bot · 2020-08-23T09:27:10Z

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

ronag

The properties are already defined below? Also would need tests.

EDIT: Something is weird here. I think you should remove the properties from Readable and like you do add them to ReadableState instead. Looks like a mistake in #31144.

lpinca · 2020-08-23T09:49:51Z

The properties are already defined below? Also would need tests.

One definition is on Readable, the other is on ReadableState. It looks like no reviewer noticed this in #31144 :/

ronag · 2020-08-23T09:50:28Z

One definition is on Readable

I think it was moved to Readable by mistake and should have been on ReadableState. Any chance we could remove it from Readable?

lpinca · 2020-08-23T09:55:48Z

I'm 👍 on keeping these only on ReadableState as it originally was before #31144.

lpinca · 2020-08-23T09:57:31Z

The change on #31144 was unintentional and broken anyway.

lundibundi · 2020-08-23T10:16:08Z

I'm good with keeping them on state only but since they were present (though broken) on Readable I thought it'd be better to leave them on Readable as well.

Though I've used .paused to remove the readable state usage in http #34888, can we replace it with something else?

ronag · 2020-08-23T10:19:38Z

Though I've used .paused to remove the readable state usage in http #34888, can we replace it with something else?

isPaused()?

lundibundi · 2020-08-23T10:20:23Z

@ronag oh, that makes sense 😄

Looks like they have been accidentally moved in nodejs#31144.

nodejs-github-bot · 2020-08-23T10:35:22Z

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

ronag · 2020-08-23T10:35:45Z

What's request-ci label? Is there some bot that looks at that and starts ci automatically?

nodejs-github-bot · 2020-08-23T10:38:52Z

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

lundibundi · 2020-08-23T10:40:13Z

@ronag yep, #34594

nodejs-github-bot · 2020-08-23T10:56:51Z

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

mcollina

lgtm

antsmartian

Looks like I missed it somehow :/

github-actions · 2020-08-24T06:10:35Z

Commit Queue failed

- Loading data for nodejs/node/pull/34886
✔ Done loading data for nodejs/node/pull/34886
----------------------------------- PR info ------------------------------------
Title stream: fix Readable stream state properties (#34886)
⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch lundibundi:fix-stream-getter-props -> nodejs:master
Labels stream
Commits 2
- stream: fix Readable stream state properties
- fixup! stream: fix Readable stream state properties
Committers 1
- Denys Otrishko PR-URL: https://github.com/nodejs/node/pull/34886
Reviewed-By: Robert Nagy Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina Reviewed-By: Zeyu Yang Reviewed-By: Anto Aravinth ------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34886
Reviewed-By: Robert Nagy Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina Reviewed-By: Zeyu Yang Reviewed-By: Anto Aravinth --------------------------------------------------------------------------------
ℹ Last Full PR CI on 2020-08-23T10:56:51Z: https://ci.nodejs.org/job/node-test-pull-request/32901/
- Querying data of job/node-test-pull-request/32901/
✔ Build data downloaded
ℹ This PR was created on Sun, 23 Aug 2020 09:24:58 GMT
✔ Approvals: 5
✔ - Robert Nagy (@ronag): https://github.com/nodejs/node/pull/34886#pullrequestreview-472999909
✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/34886#pullrequestreview-472996953
✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/34886#pullrequestreview-473001353
✔ - Zeyu Yang (@himself65): https://github.com/nodejs/node/pull/34886#pullrequestreview-473012786
✔ - Anto Aravinth (@antsmartian): https://github.com/nodejs/node/pull/34886#pullrequestreview-473059639
✖ This PR needs to wait 27 more hours to land
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

github-actions · 2020-09-01T20:39:22Z

Landed in e2ffa45

Looks like they have been accidentally moved in #31144. PR-URL: #34886 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

Looks like they have been accidentally moved in nodejs#31144. PR-URL: nodejs#34886 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

lundibundi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2020

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Aug 23, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2020

ronag requested changes Aug 23, 2020
View reviewed changes

Comment threadlib/_stream_readable.js

ronag requested changes Aug 23, 2020
View reviewed changes

lpinca approved these changes Aug 23, 2020
View reviewed changes

lundibundi mentioned this pull request Aug 23, 2020
http: remove usage of internal stream state from _http_server #34888
Closed
4 tasks

stream: fix Readable stream state properties
9cc6c1f
Looks like they have been accidentally moved in nodejs#31144.

lundibundi force-pushed the fix-stream-getter-props branch from caeb4e8 to 9cc6c1fCompare August 23, 2020 10:30

lundibundi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2020

ronag approved these changes Aug 23, 2020
View reviewed changes

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2020

lpinca reviewed Aug 23, 2020
View reviewed changes

Comment threadlib/_stream_readable.js Outdated

fixup! stream: fix Readable stream state properties
4a3f7d2

lundibundi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2020

mcollina approved these changes Aug 23, 2020
View reviewed changes

himself65 approved these changes Aug 23, 2020
View reviewed changes

antsmartian approved these changes Aug 24, 2020
View reviewed changes

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 24, 2020

github-actionsBot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 24, 2020

github-actionsBot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 24, 2020

rickyes approved these changes Aug 24, 2020
View reviewed changes

lundibundi removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 25, 2020

lundibundi added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 1, 2020

github-actionsBot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 1, 2020

github-actionsBot closed this Sep 1, 2020

lundibundi deleted the fix-stream-getter-props branch September 1, 2020 20:43

richardlau mentioned this pull request Sep 2, 2020
v14.10.0 proposal #35023
Merged
4 tasks

Uh oh!

Uh oh!

Conversation

lundibundi commented Aug 23, 2020

Checklist

Uh oh!

nodejs-github-bot commented Aug 23, 2020

Uh oh!

Uh oh!

ronag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Aug 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Aug 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Aug 23, 2020

Uh oh!

lpinca commented Aug 23, 2020

Uh oh!

lundibundi commented Aug 23, 2020

Uh oh!

ronag commented Aug 23, 2020

Uh oh!

lundibundi commented Aug 23, 2020

Uh oh!

nodejs-github-bot commented Aug 23, 2020

Uh oh!

ronag commented Aug 23, 2020

Uh oh!

nodejs-github-bot commented Aug 23, 2020

Uh oh!

lundibundi commented Aug 23, 2020

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 23, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

antsmartian left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBot commented Aug 24, 2020

Uh oh!

github-actionsBot commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ronag left a comment •
edited
Loading

lpinca commented Aug 23, 2020•
edited
Loading

ronag commented Aug 23, 2020•
edited
Loading