★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/24128Nouvelle 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

net: some scattered cleanup #24128

Closed
wants to merge 1 commit into from
+17 −9

Conversation

Reviewers

@trygve-lie trygve-lie

@Trott Trott

@vsemozhetbyt vsemozhetbyt

@addaleax addaleax

@lpinca lpinca

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
author ready net
Projects
None yet
Milestone
No milestone
9 participants
@oyyd @refack @lpinca @BridgeAR @trygve-lie @Trott @addaleax @vsemozhetbyt @nodejs-github-bot
@oyyd
Copy link
Contributor

oyyd commented Nov 6, 2018

This commit cleans up net module, including:

  • Remove assigning handle.readable and handle.writable. It's add in nodejs/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

@nodejs-github-bot nodejs-github-bot added the net label Nov 6, 2018

@addaleax

addaleax approved these changes Nov 6, 2018
View changes

doc/api/cli.md Outdated
@@ -751,6 +751,11 @@ threadpool by setting the `'UV_THREADPOOL_SIZE'` environment variable to a value
greater than `4` (its current default value). For more information, see the
[libuv threadpool documentation][].

### `NODE_PENDING_PIPE_INSTANCES=instances`

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Nov 6, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@vsemozhetbyt

vsemozhetbyt Nov 6, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@oyyd

oyyd Nov 7, 2018

Author Contributor

@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 oyyd force-pushed the oyyd:net-cleanup branch from fb67ac0 to 3f2cb41 Nov 7, 2018

@oyyd

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

oyyd commented Nov 12, 2018

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

@oyyd oyyd force-pushed the oyyd:net-cleanup branch 3 times, most recently from 373e89f to 1187d2b Nov 15, 2018

@oyyd

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

oyyd commented Nov 15, 2018

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

Trott reviewed Nov 15, 2018
View changes

doc/api/cli.md Outdated
### `NODE_PENDING_PIPE_INSTANCES=instances`

Set the number of pending pipe instance handles when the pipe server is waiting
for connections. Note that this setting applies to Windows only.

This comment has been minimized.

Sign in to view
Copy link
@Trott

Trott Nov 15, 2018

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

This comment has been minimized.

Sign in to view
Copy link
@oyyd

oyyd Nov 15, 2018

Author Contributor

Resolved.

@oyyd oyyd force-pushed the oyyd:net-cleanup branch from 1187d2b to a6a109e Nov 15, 2018

@oyyd

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

oyyd commented Nov 15, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Nov 15, 2018

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

@oyyd oyyd force-pushed the oyyd:net-cleanup branch from a6a109e to 785e551 Nov 22, 2018

@oyyd

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

oyyd commented Nov 22, 2018

Any other thoughts on this?
@oyyd

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

oyyd commented Nov 22, 2018 •
edited

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

trygve-lie reviewed Nov 22, 2018
View changes

lib/net.js Outdated
rval = createServerHandle('::', port, 6, fd, flags);
=======
rval = createServerHandle(DEFAULT_IPV6_ADDR, port, 6, fd);
>>>>>>> a6a109ee94... net: some scattered cleanup

This comment has been minimized.

Sign in to view
Copy link
@trygve-lie

trygve-lie Nov 22, 2018

Are these merge conflicts supposed to be here?

This comment has been minimized.

Sign in to view
Copy link
@oyyd

oyyd Nov 22, 2018

Author Contributor

No, my bad.

@oyyd oyyd force-pushed the oyyd:net-cleanup branch from 785e551 to cba0667 Nov 22, 2018

@lpinca

This comment has been minimized.

Sign in to view
Copy link
Member

lpinca commented Mar 13, 2019

@oyyd can you rebase?
@lpinca

lpinca approved these changes Mar 13, 2019
View changes

@oyyd oyyd force-pushed the oyyd:net-cleanup branch 2 times, most recently from 58c078e to 8dc83d5 Mar 13, 2019

@oyyd

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

oyyd commented Mar 13, 2019

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

@lpinca Rebased.
@lpinca

This comment has been minimized.

Sign in to view
Copy link
Member

lpinca commented Mar 13, 2019

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

@refack refack added the author ready label Mar 13, 2019

@BridgeAR

BridgeAR approved these changes Mar 13, 2019
View changes

@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

BridgeAR commented Mar 13, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

lpinca commented Mar 14, 2019

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/21536/
@oyyd
net: some scattered cleanup 
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 '::'.
Loading status checks…
90ee8de

@oyyd oyyd closed this Mar 14, 2019

@oyyd oyyd force-pushed the oyyd:net-cleanup branch from 8dc83d5 to 107c95d Mar 14, 2019

@oyyd oyyd reopened this Mar 14, 2019

@oyyd

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

oyyd commented Mar 14, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Mar 14, 2019

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

oyyd added a commit that referenced this pull request Mar 15, 2019

@oyyd
net: some scattered cleanup 
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>
Loading status checks…
cd8b739
@oyyd

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

oyyd commented Mar 15, 2019

Landed in cd8b739. Thank you all!

@oyyd oyyd closed this Mar 15, 2019

Drieger added a commit to Drieger/node that referenced this pull request Mar 22, 2019

@oyyd @Drieger
net: some scattered cleanup 
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>
32b99a2

targos added a commit to targos/node that referenced this pull request Mar 27, 2019

@oyyd @targos
net: some scattered cleanup 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
da4601b

targos added a commit to targos/node that referenced this pull request Mar 27, 2019

@oyyd @targos
net: some scattered cleanup 
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>
31d58b6

targos added a commit to targos/node that referenced this pull request Mar 27, 2019

@oyyd @targos
net: some scattered cleanup 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
674765d

targos added a commit that referenced this pull request Mar 27, 2019

@oyyd @targos
net: some scattered cleanup 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
24e96b2

@targos targos referenced this pull request Mar 27, 2019

Merged

v11.13.0 release proposal #26949

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.