net: some scattered cleanup by oyyd · Pull Request #24128 · nodejs/node

oyyd · 2018-11-06T12:58:42Z

This commit cleans up net module, including:

Remove assigning handle.readable and handle.writable. It's add in Server.listen({ fd: number }) node-v0.x-archive#3422 and I see nowhere we use these two properties right now.
Documents the enviroment variable of NODE_PENDING_PIPE_INSTANCES. It was add in 99c9d19 long time ago but was not documented and there is no test for it. Maybe we can consider removing it?
Use constants for '0.0.0.0' and '::'.

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

addaleax · 2018-11-06T15:01:44Z

Can you add an YAML added: block, similar to other items in this file?

Also: it seems these sections are sorted alphabetically, so this one needs to be placed after the NODE_PENDING_DEPRECATION=1.

@addaleax Though NODE_PENDING_DEPRECATION could be documented here, it's "added" long time ago rather than a new feature. Maybe we should keep the document here without added: block?
Also: it seems these sections are sorted alphabetically, so this one needs to be placed after the NODE_PENDING_DEPRECATION=1.
Done!

oyyd · 2018-11-12T10:25:38Z

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

oyyd · 2018-11-15T13:57:57Z

Resume: https://ci.nodejs.org/job/node-test-pull-request/18645/

Trott · 2018-11-15T14:02:12Z

Suggested change
for connections. Note that this setting applies to Windows only.
for connections. This setting applies to Windows only.

oyyd · 2018-11-15T14:31:53Z

Resume: https://ci.nodejs.org/job/node-test-pull-request/18646/

Can you add an YAML added: block, similar to other items in this file?

@addaleax I see that #24289 documents NODE_TLS_REJECT_UNAUTHORIZED without the added: block so that I believe this should be okay.

And does this still LGTY?

refack · 2018-11-15T14:32:04Z

Sorry about the node-test-linux-linked-withoutssl. I'm working on adding this, and it's just not stable yet.

oyyd · 2018-11-22T13:52:28Z

Any other thoughts on this?

oyyd · 2018-11-22T13:53:48Z

Resume for rebasing: ~~https://ci.nodejs.org/job/node-test-pull-request/18882/~~
https://ci.nodejs.org/job/node-test-pull-request/18884/

trygve-lie · 2018-11-22T13:57:30Z

Are these merge conflicts supposed to be here?

No, my bad.

lpinca · 2019-03-13T08:36:34Z

@oyyd can you rebase?

oyyd · 2019-03-13T13:30:28Z

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

@lpinca Rebased.

lpinca · 2019-03-13T13:33:08Z

Thank you. I think you can land this after a green CI.

BridgeAR · 2019-03-13T15:33:04Z

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

lpinca · 2019-03-14T07:22:16Z

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

This commit cleans up net module, including: 1. remove assigning `handle.readable` and `handle.writable` 2. documents `NODE_PENDING_PIPE_INSTANCES` enviroment variable 3. use constants for '0.0.0.0' and '::'.

oyyd · 2019-03-14T11:22:08Z

The tests on node-test-binary-arm are failing continuously. I have merged the master and resume the CI again. (Sorry for closing the PR unexpectly).

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

refack · 2019-03-14T15:06:53Z

Probably was a transient issue - https://ci.nodejs.org/job/node-test-commit-arm-fanned/7205/

This commit cleans up net module, including: 1. remove assigning `handle.readable` and `handle.writable` 2. documents `NODE_PENDING_PIPE_INSTANCES` enviroment variable 3. use constants for '0.0.0.0' and '::'. PR-URL: #24128 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

oyyd · 2019-03-15T02:36:06Z

Landed in cd8b739. Thank you all!

This commit cleans up net module, including: 1. remove assigning `handle.readable` and `handle.writable` 2. documents `NODE_PENDING_PIPE_INSTANCES` enviroment variable 3. use constants for '0.0.0.0' and '::'. PR-URL: nodejs#24128 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This commit cleans up net module, including: 1. remove assigning `handle.readable` and `handle.writable` 2. documents `NODE_PENDING_PIPE_INSTANCES` enviroment variable 3. use constants for '0.0.0.0' and '::'. PR-URL: #24128 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Nov 6, 2018

addaleax approved these changes Nov 6, 2018
View reviewed changes

oyyd force-pushed the net-cleanup branch from fb67ac0 to 3f2cb41Compare November 7, 2018 02:51

oyyd force-pushed the net-cleanup branch 3 times, most recently from 373e89f to 1187d2bCompare November 15, 2018 12:30

Trott reviewed Nov 15, 2018
View reviewed changes

oyyd force-pushed the net-cleanup branch from 1187d2b to a6a109eCompare November 15, 2018 14:16

oyyd force-pushed the net-cleanup branch from a6a109e to 785e551Compare November 22, 2018 13:49

trygve-lie reviewed Nov 22, 2018
View reviewed changes

oyyd force-pushed the net-cleanup branch from 785e551 to cba0667Compare November 22, 2018 14:04

lpinca approved these changes Mar 13, 2019
View reviewed changes

oyyd force-pushed the net-cleanup branch 2 times, most recently from 58c078e to 8dc83d5Compare March 13, 2019 13:20

refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2019

BridgeAR approved these changes Mar 13, 2019
View reviewed changes

net: some scattered cleanup
90ee8de
This commit cleans up net module, including: 1. remove assigning `handle.readable` and `handle.writable` 2. documents `NODE_PENDING_PIPE_INSTANCES` enviroment variable 3. use constants for '0.0.0.0' and '::'.

oyyd closed this Mar 14, 2019

oyyd force-pushed the net-cleanup branch from 8dc83d5 to 107c95dCompare March 14, 2019 11:15

oyyd reopened this Mar 14, 2019

oyyd closed this Mar 15, 2019

targos mentioned this pull request Mar 27, 2019
v11.13.0 release proposal #26949
Merged

	for connections. Note that this setting applies to Windows only.
	for connections. This setting applies to Windows only.

Uh oh!

Uh oh!

Conversation

oyyd commented Nov 6, 2018

Checklist

Uh oh!

addaleaxNov 6, 2018

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytNov 6, 2018

Choose a reason for hiding this comment

Uh oh!

oyydNov 7, 2018

Choose a reason for hiding this comment

Uh oh!

oyyd commented Nov 12, 2018

Uh oh!

oyyd commented Nov 15, 2018

Uh oh!

TrottNov 15, 2018

Choose a reason for hiding this comment

Uh oh!

oyydNov 15, 2018

Choose a reason for hiding this comment

Uh oh!

oyyd commented Nov 15, 2018

Uh oh!

refack commented Nov 15, 2018

Uh oh!

oyyd commented Nov 22, 2018

Uh oh!

oyyd commented Nov 22, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trygve-lieNov 22, 2018

Choose a reason for hiding this comment

Uh oh!

oyydNov 22, 2018

Choose a reason for hiding this comment

Uh oh!

lpinca commented Mar 13, 2019

Uh oh!

oyyd commented Mar 13, 2019

Uh oh!

lpinca commented Mar 13, 2019

Uh oh!

BridgeAR commented Mar 13, 2019

Uh oh!

lpinca commented Mar 14, 2019

Uh oh!

oyyd commented Mar 14, 2019

Uh oh!

refack commented Mar 14, 2019

Uh oh!

oyyd commented Mar 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

oyyd commented Nov 22, 2018•
edited
Loading