★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/28007Nouvelle recherche | Portail wanayoo
Skip to content

/node

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
+93 −4

Conversation

Reviewers

@mcollina mcollina

@BridgeAR BridgeAR

@Himself65 Himself65

@jasnell jasnell

@Trott Trott

@addaleax addaleax

@lpinca lpinca

Assignees
No one assigned
Labels
author ready http2 semver-minor stream
Projects
None yet
Milestone
No milestone
9 participants
@zero1five @lpinca @Himself65 @nodejs-github-bot @mcollina @addaleax @Trott @jasnell @BridgeAR
@zero1five
Copy link
Contributor

commented Jun 1, 2019

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 stream labels Jun 1, 2019

@lpinca

lpinca reviewed Jun 1, 2019
View changes

Show resolved Hide resolved lib/_stream_duplex.js
@Himself65

Himself65 suggested changes Jun 2, 2019
View changes

Show resolved Hide resolved lib/_stream_duplex.js
@zero1five

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

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?

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

This comment has been minimized.

Sign in to view
Copy link
Member

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

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 2, 2019

@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 #27974

What do you think?
@zero1five

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 2, 2019 •
edited

@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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 2, 2019 •
edited

@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 zero1five:refactor/replace-finished branch from 868f2f1 to 6ce51da Jun 2, 2019

@zero1five

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 2, 2019

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

lpinca approved these changes Jun 2, 2019
View changes

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 2, 2019

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

@zero1five zero1five force-pushed the zero1five:refactor/replace-finished branch from 6ce51da to cdb22c4 Jun 2, 2019

@zero1five

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 2, 2019

@lpinca 👌
@Himself65

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jun 2, 2019

LGTM
@BridgeAR

BridgeAR reviewed Jun 4, 2019
View changes

Copy link
Member

left a comment

This needs some documentation. Otherwise it's LGTM.

@BridgeAR BridgeAR added the semver-minor label Jun 4, 2019

@zero1five zero1five force-pushed the zero1five:refactor/replace-finished branch 2 times, most recently from d54a111 to de53445 Jun 11, 2019

@zero1five

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 11, 2019

@BridgeAR Added the doc, PTAL.

@jasnell jasnell requested a review from mcollina Jun 14, 2019

@jasnell

jasnell approved these changes Jun 14, 2019
View changes

@mcollina

mcollina reviewed Jun 14, 2019
View changes

Copy link
Member

left a comment

Good work! I’ve just added a nit on the doc!
Show resolved Hide resolved doc/api/stream.md Outdated

@zero1five zero1five force-pushed the zero1five:refactor/replace-finished branch 3 times, most recently from f1d35d4 to 12622a5 Jun 14, 2019

@zero1five

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 19, 2019

Can someone please trigger a CI build?
@addaleax

addaleax approved these changes Jun 19, 2019
View changes

@addaleax addaleax added the author ready label Jun 19, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Jun 21, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/24035/
@Trott

Trott approved these changes Jun 21, 2019
View changes

@zero1five

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jun 24, 2019

Windows-fanned has one use case that fails, test.parallel/test-http2-client-destroy...

zero1five added some commits Jun 1, 2019

@zero1five
stream: add writableFinished 
add a new getter to duplex stream to replace the property `this
.writableState.finished` of the object that inherited duplex.

Refs: #445
5cd6ccc
@zero1five
http2: use writableFinished instead of _writableState
Loading status checks…
2c66f6c

@zero1five zero1five force-pushed the zero1five:refactor/replace-finished branch from 284f714 to 2c66f6c Jun 24, 2019

@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Jun 24, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 25, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 25, 2019

Landed in 2bb93e1...f11a4ec

@Trott Trott closed this Jun 25, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jun 25, 2019

@zero1five @Trott
stream: add writableFinished 
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>
33aef82

Trott added a commit to Trott/io.js that referenced this pull request Jun 25, 2019

@zero1five @Trott
http2: use writableFinished instead of _writableState 
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>
f11a4ec

@Trott Trott referenced this pull request Jun 25, 2019

Merged

net: replace _writableState with writableFinished #27974

3 of 3 tasks complete

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

@cjihrig
doc: replace version with REPLACEME 
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
Verified
This commit was signed with a verified signature.
cjihrig Colin Ihrig
GPG key ID: 7434390BDBE9B9C5 Learn about signing commits
22b992b

@cjihrig cjihrig referenced this pull request Jun 26, 2019

Closed

doc: replace version with REPLACEME #28431

3 of 3 tasks complete

@zero1five zero1five deleted the zero1five:refactor/replace-finished branch Jun 26, 2019

danbev added a commit that referenced this pull request Jun 26, 2019

@cjihrig @danbev
doc: replace version with REPLACEME 
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>
Loading status checks…
5b4a0cb

targos added a commit that referenced this pull request Jul 2, 2019

@zero1five @targos
stream: add writableFinished 
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>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
1650bcf

targos added a commit that referenced this pull request Jul 2, 2019

@zero1five @targos
http2: use writableFinished instead of _writableState 
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>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
d8942f8

targos added a commit that referenced this pull request Jul 2, 2019

@cjihrig @targos
doc: replace version with REPLACEME 
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>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
5367d02

targos added a commit that referenced this pull request Jul 2, 2019

@targos
2019-07-03, Version 12.6.0 (Current) 
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
Loading status checks…
d711542

@targos targos referenced this pull request Jul 2, 2019

Merged

Release v12.6.0 proposal #28508

targos added a commit that referenced this pull request Jul 2, 2019

@targos
2019-07-03, Version 12.6.0 (Current) 
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
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
e6a8ac1

targos added a commit that referenced this pull request Jul 2, 2019

@targos
2019-07-03, Version 12.6.0 (Current) 
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
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
e71a0f4

targos added a commit that referenced this pull request Jul 3, 2019

@targos
2019-07-03, Version 12.6.0 (Current) 
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
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
Loading status checks…
fcf8fe9

This was referenced Jul 3, 2019

Closed
Closed
Open

@ehmicky ehmicky referenced this pull request Jul 4, 2019

Merged

Fix test for Node `>= 12.6.0` #348

@vsemozhetbyt vsemozhetbyt referenced this pull request Jul 8, 2019

Closed

doc: fix nits in stream.md #28591

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.