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

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

Closed
wants to merge 2 commits into from

Conversation

zero1five
Copy link
Contributor

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

@nodejs-github-bot 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
lib/_stream_duplex.js Show resolved Hide resolved
@zero1five
Copy link
Contributor Author

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.

@lpinca
Copy link
Member

lpinca commented Jun 2, 2019

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

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.

@lpinca
Copy link
Member

lpinca commented Jun 2, 2019

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

What do you think?

@zero1five
Copy link
Contributor Author

zero1five commented Jun 2, 2019

@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?)

@lpinca
Copy link
Member

lpinca commented Jun 2, 2019

@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.

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

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).

test/parallel/test-stream-duplex-writable-finished.js Outdated Show resolved Hide resolved
test/parallel/test-stream-writable-finished.js Outdated Show resolved Hide resolved
@lpinca
Copy link
Member

lpinca commented Jun 2, 2019

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

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

@lpinca 👌

@himself65
Copy link
Member

LGTM

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs some documentation. Otherwise it's LGTM.

@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 4, 2019
@zero1five zero1five force-pushed the refactor/replace-finished branch 2 times, most recently from d54a111 to de53445 Compare June 11, 2019 15:03
@zero1five
Copy link
Contributor Author

@BridgeAR Added the doc, PTAL.

@jasnell jasnell requested a review from mcollina June 14, 2019 03:09
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/stream.md Outdated Show resolved Hide resolved
@zero1five zero1five force-pushed the refactor/replace-finished branch 3 times, most recently from f1d35d4 to 12622a5 Compare June 19, 2019 21:32
@zero1five
Copy link
Contributor Author

Can someone please trigger a CI build?

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 19, 2019
@zero1five zero1five force-pushed the refactor/replace-finished branch from 284f714 to 2c66f6c Compare June 24, 2019 13:31
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

@Trott
Copy link
Member

Trott commented Jun 25, 2019

Landed in 2bb93e1...f11a4ec

@Trott Trott closed this Jun 25, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 25, 2019
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 25, 2019
PR-URL: nodejs#28007
Refs: nodejs#445
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
cjihrig added a commit to cjihrig/node that referenced this pull request Jun 26, 2019
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
@zero1five zero1five deleted the refactor/replace-finished branch June 26, 2019 02:59
danbev pushed a commit that referenced this pull request Jun 26, 2019
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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit that referenced this pull request Jul 2, 2019
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jul 2, 2019
PR-URL: #28007
Refs: #445
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jul 2, 2019
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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos added a commit that referenced this pull request Jul 2, 2019
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
@targos targos mentioned this pull request Jul 2, 2019
targos added a commit that referenced this pull request Jul 2, 2019
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
targos added a commit that referenced this pull request Jul 2, 2019
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
targos added a commit that referenced this pull request Jul 3, 2019
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
@vsemozhetbyt vsemozhetbyt mentioned this pull request Jul 8, 2019
3 tasks
@BethGriggs
Copy link
Member

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

@mcollina
Copy link
Member

mcollina commented Sep 4, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants