http2: use getter replace use directly _witableState.finished by zero1five · Pull Request #28007 · nodejs/node · GitHub

add a new getter to duplex stream to replace the property this .writableState.finished of the object that inherited duplex.

Refs: #445

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

It's worth noting that Duplex already has some getters for _writableState. Does this need to be moved to writable.prototype?

I'm also looking for a way to proxy all _writableState properties at once, without losing readability and conciseness.

It's worth noting that Duplex already has some getters for _writableState. Does this need to be moved to writable.prototype?

They are also on Writable.prototype with the exact same definition. This is because Duplex inherits from both Readable and Writable but in this loop

node/lib/_stream_duplex.js

Lines 42 to 46 in 187b056

    
           for (var v = 0; v < keys.length; v++) { 
        
             const method = keys[v]; 
        
             if (!Duplex.prototype[method]) 
        
               Duplex.prototype[method] = Writable.prototype[method]; 
        
           }

non enumerable properties are skipped, so we have to redefine them.

@zero1five I think this PR should be split in two logical changes/commits:

One that adds the writableLength getter to both Writable.prototype and Duplex.prototype with relevant tests.
One that uses the new getter in lib/internal/http2/core.js and possibly other places like net: replace _writableState with writableFinished #27974

What do you think?

@lpinca Ok, I feel good. i will update this PR later to update this section with relevant tests. And then see what else is being used.

(a little curiosity 🗿, why does Duplex prototype set skip non enumerable properties?)

@zero1five I'm not sure, I think that with a little care we can use Object.getOwnPropertyNames() instead of Object.keys() and avoid redefining most of those getters in Duplex.

Hmmm...🏊‍♂️ i made some changes to add getter with relevant tests for Duplex and Writable.
Then I looked under ./lib for the use of the new getter:

lib/net.js(should be in #27974 complete.)
lib/internal/http2/core.js
other places...

Because this PR is mainly to modify http2, so other changes should be modified in the new PR.
@lpinca @Trott @himself65 PTAL(should also @ who).

LGTM but can you please move the http2 change to a separate commit? Thank you.

@lpinca 👌

LGTM

This needs some documentation. Otherwise it's LGTM.

@BridgeAR Added the doc, PTAL.

Good work! I’ve just added a nit on the doc!

Can someone please trigger a CI build?

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

Windows re-build: https://ci.nodejs.org/job/node-test-commit-windows-fanned/28330/

Landed in 2bb93e1...f11a4ec

add a new getter to duplex stream to replace the property `this .writableState.finished` of the object that inherited duplex. Refs: nodejs#445 PR-URL: nodejs#28007 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#28007 Refs: nodejs#445 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

Node 12.4.0 has already been released. Replace the version with REPLACEME so that the proper version gets inserted at release time. Refs: nodejs#28007

Node 12.4.0 has already been released. Replace the version with REPLACEME so that the proper version gets inserted at release time. PR-URL: #28431 Refs: #28007 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

add a new getter to duplex stream to replace the property `this .writableState.finished` of the object that inherited duplex. Refs: #445 PR-URL: #28007 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #28007 Refs: #445 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>

Node 12.4.0 has already been released. Replace the version with REPLACEME so that the proper version gets inserted at release time. PR-URL: #28431 Refs: #28007 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

Notable changes: * build: * Experimental support for building Node.js on MIPS architecture is back. #27992 * child_process: * The promisified versions of `child_process.exec` and `child_process.execFile` now both return a `Promise` which has the child instance attached to their `child` property. #28325 * deps: * Updated libuv to 1.30.0. #28449 * Support for the Haiku platform has been added. * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to 1024. * `uv_fs_copyfile()` now works properly when the source and destination files are the same. * process: * A new method, `process.resourceUsage()` was added. It returns resource usage for the current process, such as CPU time. #28018 * src: * Fixed an issue related to stdio that could lead to a crash of the process in some circumstances. #28490 * stream: * Added a `writableFinished` property to writable streams. It indicates that all the data has been flushed to the underlying system. #28007 * worker: * Fixed an issue that prevented worker threads to listen for data on stdin. #28153 PR-URL: #28508

Notable changes: * build: * Experimental support for building Node.js on MIPS architecture is back. #27992 * child_process: * The promisified versions of `child_process.exec` and `child_process.execFile` now both return a `Promise` which has the child instance attached to their `child` property. #28325 * deps: * Updated libuv to 1.30.0. #28449 * Support for the Haiku platform has been added. * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to 1024. * `uv_fs_copyfile()` now works properly when the source and destination files are the same. * process: * A new method, `process.resourceUsage()` was added. It returns resource usage for the current process, such as CPU time. #28018 * src: * Fixed an issue related to stdio that could lead to a crash of the process in some circumstances. #28490 * stream: * Added a `writableFinished` property to writable streams. It indicates that all the data has been flushed to the underlying system. #28007 * worker: * Fixed an issue that prevented worker threads to listen for data on stdin. #28153 * meta: * Added Jiawen Geng (https://github.com/gengjiawen) to collaborators. #28322 PR-URL: #28508

Notable changes: * build: * Experimental support for building Node.js on MIPS architecture is back. #27992 * child_process: * The promisified versions of `child_process.exec` and `child_process.execFile` now both return a `Promise` which has the child instance attached to their `child` property. #28325 * deps: * Updated libuv to 1.30.1. #28449, #28511 * Support for the Haiku platform has been added. * The maximum `UV_THREADPOOL_SIZE` has been increased from 128 to 1024. * `uv_fs_copyfile()` now works properly when the source and destination files are the same. * process: * A new method, `process.resourceUsage()` was added. It returns resource usage for the current process, such as CPU time. #28018 * src: * Fixed an issue related to stdio that could lead to a crash of the process in some circumstances. #28490 * stream: * Added a `writableFinished` property to writable streams. It indicates that all the data has been flushed to the underlying system. #28007 * worker: * Fixed an issue that prevented worker threads to listen for data on stdin. #28153 * meta: * Added Jiawen Geng (https://github.com/gengjiawen) to collaborators. #28322 PR-URL: #28508

@nodejs/http2, should this land on v10.x? Please add the 'lts-watch' label if so

@BethGriggs yes please, I've added the tag.

nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. stream Issues and PRs related to the stream subsystem. labels Jun 1, 2019

lpinca reviewed Jun 1, 2019

View reviewed changes

Comment thread lib/_stream_duplex.js Outdated

himself65 requested changes Jun 2, 2019

View reviewed changes

Comment thread lib/_stream_duplex.js Outdated

zero1five force-pushed the refactor/replace-finished branch from 868f2f1 to 6ce51da Compare June 2, 2019 09:50

lpinca approved these changes Jun 2, 2019

View reviewed changes

Comment thread test/parallel/test-stream-duplex-writable-finished.js Outdated

Comment thread test/parallel/test-stream-writable-finished.js Outdated

zero1five force-pushed the refactor/replace-finished branch from 6ce51da to cdb22c4 Compare June 2, 2019 12:03

BridgeAR reviewed Jun 4, 2019

View reviewed changes

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 4, 2019

zero1five force-pushed the refactor/replace-finished branch 2 times, most recently from d54a111 to de53445 Compare June 11, 2019 15:03

jasnell requested a review from mcollina June 14, 2019 03:09

jasnell approved these changes Jun 14, 2019

View reviewed changes

mcollina reviewed Jun 14, 2019

View reviewed changes

Comment thread doc/api/stream.md Outdated

zero1five force-pushed the refactor/replace-finished branch 3 times, most recently from f1d35d4 to 12622a5 Compare June 19, 2019 21:32

addaleax approved these changes Jun 19, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 19, 2019

http2: use writableFinished instead of _writableState

2c66f6c

zero1five force-pushed the refactor/replace-finished branch from 284f714 to 2c66f6c Compare June 24, 2019 13:31

Trott closed this Jun 25, 2019

Trott mentioned this pull request Jun 25, 2019

net: replace _writableState with writableFinished #27974

Merged

3 tasks

cjihrig added a commit to cjihrig/node that referenced this pull request Jun 26, 2019

doc: replace version with REPLACEME

22b992b

Node 12.4.0 has already been released. Replace the version with REPLACEME so that the proper version gets inserted at release time. Refs: nodejs#28007

cjihrig mentioned this pull request Jun 26, 2019

doc: replace version with REPLACEME #28431

Closed

3 tasks

zero1five deleted the refactor/replace-finished branch June 26, 2019 02:59

targos mentioned this pull request Jul 2, 2019

Release v12.6.0 proposal #28508

Merged

This was referenced Jul 3, 2019

2019-07-03 Version 12.6.0 (Current) @targos beevelop/docker-nodejs#288

Closed

2019-07-03 Version 12.6.0 (Current) @targos beevelop/docker-nodejs#289

Closed

2019-07-03 Version 12.6.0 (Current) @targos beevelop/docker-nodejs#290

Closed

ehmicky mentioned this pull request Jul 4, 2019

Fix test for Node >= 12.6.0 sindresorhus/execa#348

Merged

vsemozhetbyt mentioned this pull request Jul 8, 2019

doc: fix nits in stream.md #28591

Closed

3 tasks

mcollina added the lts-watch-v10.x label Sep 4, 2019

BethGriggs added the backport-requested-v10.x label Oct 17, 2019

http2: use getter replace use directly _witableState.finished by zero1five · Pull Request #28007 · nodejs/node · GitHub

Uh oh!

Uh oh!

http2: use getter replace use directly _witableState.finished - #28007

http2: use getter replace use directly _witableState.finished#28007zero1five wants to merge 2 commits intonodejs:masternodejs/node:masterfrom zero1five:refactor/replace-finishedzero1five/node:refactor/replace-finishedCopy head branch name to clipboard

Conversation

zero1five commented Jun 1, 2019

Checklist

Uh oh!

Uh oh!

Uh oh!

zero1five commented Jun 2, 2019

Uh oh!

lpinca commented Jun 2, 2019

Uh oh!

lpinca commented Jun 2, 2019

Uh oh!

zero1five commented Jun 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Jun 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zero1five commented Jun 2, 2019

Uh oh!

Uh oh!

Uh oh!

lpinca commented Jun 2, 2019

Uh oh!

zero1five commented Jun 2, 2019

Uh oh!

himself65 commented Jun 2, 2019

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

zero1five commented Jun 11, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zero1five commented Jun 19, 2019

Uh oh!

nodejs-github-bot commented Jun 24, 2019

Uh oh!

addaleax commented Jun 25, 2019

Uh oh!

Trott commented Jun 25, 2019

Uh oh!

BethGriggs commented Sep 3, 2019

Uh oh!

mcollina commented Sep 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

http2: use getter replace use directly _witableState.finished#28007
zero1five wants to merge 2 commits into
nodejs:masterfrom
zero1five:refactor/replace-finished

zero1five commented Jun 2, 2019 •
edited
Loading

lpinca commented Jun 2, 2019 •
edited
Loading