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.

Sign up for GitHub

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

Jump to bottom

net: check autoSelectFamilyAttemptTimeout is positive #45740

Closed
wants to merge 3 commits into from
Closed

net: check autoSelectFamilyAttemptTimeout is positive #45740

wants to merge 3 commits into from

Conversation

deokjinkim
Copy link
Contributor

@deokjinkim deokjinkim commented Dec 5, 2022 •
edited
Loading

In document, autoSelectFamilyAttemptTimeout is described as positive integer because it's time unit. But there is no checking whether it's positive integer.

Refs: https://github.com/nodejs/node/blob/main/doc/api/net.md#socketconnectoptions-connectlistener

@deokjinkim
net: check autoSelectFamilyAttemptTimeout is positive
c16edf4 
In document, `autoSelectFamilyAttemptTimeout` is described as
positive integer because it's time unit. But there is no checking
whether it's positive integer.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Dec 5, 2022
mcollina
mcollina approved these changes Dec 5, 2022
View reviewed changes
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.

lgtm

Could you add a test for this case?

ShogunPanda
ShogunPanda approved these changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

Thanks for spotting it! It looks good to me.
As @mcollina asked, can you please add a small test about it?

@deokjinkim deokjinkim force-pushed the 221205_check_positive branch from 1e767c8 to 9e89229 Compare December 5, 2022 10:36
@deokjinkim
Copy link
Contributor Author

Added test case for this PR. Please let me know if this PR needs change.

ShogunPanda
ShogunPanda approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

ShogunPanda pushed a commit that referenced this pull request Dec 8, 2022
@deokjinkim @ShogunPanda
net: check autoSelectFamilyAttemptTimeout is positive
3f98be9 
In document, `autoSelectFamilyAttemptTimeout` is described as
positive integer because it's time unit. But there is no checking
whether it's positive integer.

PR-URL: #45740
Refs: https://github.com/nodejs/node/blob/main/doc/api/net.md#socketconnectoptions-connectlistener
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@ShogunPanda
Copy link
Contributor

Landed in 3f98be9

@ShogunPanda ShogunPanda closed this Dec 8, 2022
ErickWendel pushed a commit to ErickWendel/node that referenced this pull request Dec 12, 2022
@deokjinkim @ErickWendel
net: check autoSelectFamilyAttemptTimeout is positive
afb893e 
In document, `autoSelectFamilyAttemptTimeout` is described as
positive integer because it's time unit. But there is no checking
whether it's positive integer.

PR-URL: nodejs#45740
Refs: https://github.com/nodejs/node/blob/main/doc/api/net.md#socketconnectoptions-connectlistener
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Dec 12, 2022
@deokjinkim @targos
net: check autoSelectFamilyAttemptTimeout is positive
51e367f 
In document, `autoSelectFamilyAttemptTimeout` is described as
positive integer because it's time unit. But there is no checking
whether it's positive integer.

PR-URL: #45740
Refs: https://github.com/nodejs/node/blob/main/doc/api/net.md#socketconnectoptions-connectlistener
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos mentioned this pull request Dec 12, 2022
Merged
targos pushed a commit that referenced this pull request Dec 13, 2022
@deokjinkim @targos
net: check autoSelectFamilyAttemptTimeout is positive
b9c2fc7 
In document, `autoSelectFamilyAttemptTimeout` is described as
positive integer because it's time unit. But there is no checking
whether it's positive integer.

PR-URL: #45740
Refs: https://github.com/nodejs/node/blob/main/doc/api/net.md#socketconnectoptions-connectlistener
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@deokjinkim @danielleadams
net: check autoSelectFamilyAttemptTimeout is positive
222483c 
In document, `autoSelectFamilyAttemptTimeout` is described as
positive integer because it's time unit. But there is no checking
whether it's positive integer.

PR-URL: #45740
Refs: https://github.com/nodejs/node/blob/main/doc/api/net.md#socketconnectoptions-connectlistener
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@deokjinkim @danielleadams
net: check autoSelectFamilyAttemptTimeout is positive
ccb3e46 
In document, `autoSelectFamilyAttemptTimeout` is described as
positive integer because it's time unit. But there is no checking
whether it's positive integer.

PR-URL: #45740
Refs: https://github.com/nodejs/node/blob/main/doc/api/net.md#socketconnectoptions-connectlistener
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@deokjinkim @danielleadams
net: check autoSelectFamilyAttemptTimeout is positive
a1831da 
In document, `autoSelectFamilyAttemptTimeout` is described as
positive integer because it's time unit. But there is no checking
whether it's positive integer.

PR-URL: #45740
Refs: https://github.com/nodejs/node/blob/main/doc/api/net.md#socketconnectoptions-connectlistener
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
@deokjinkim @danielleadams
net: check autoSelectFamilyAttemptTimeout is positive
c6008b6 
In document, `autoSelectFamilyAttemptTimeout` is described as
positive integer because it's time unit. But there is no checking
whether it's positive integer.

PR-URL: #45740
Refs: https://github.com/nodejs/node/blob/main/doc/api/net.md#socketconnectoptions-connectlistener
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
@deokjinkim @danielleadams
net: check autoSelectFamilyAttemptTimeout is positive
5289edc 
In document, `autoSelectFamilyAttemptTimeout` is described as
positive integer because it's time unit. But there is no checking
whether it's positive integer.

PR-URL: #45740
Refs: https://github.com/nodejs/node/blob/main/doc/api/net.md#socketconnectoptions-connectlistener
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@lpinca lpinca lpinca approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@deokjinkim @nodejs-github-bot @ShogunPanda @mcollina @lpinca @tniessen