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

net: fix family autoselection timeout handling. #47860

Merged
merged 2 commits into from
May 11, 2023

Conversation

ShogunPanda
Copy link
Contributor

@ShogunPanda ShogunPanda commented May 4, 2023

This PR fixes timeout handling of the family autoselection.

@nodejs/net @nodejs/tsc To ensure better user experience, I disabled the attempt timeout when connecting to the last IP address available, otherwise each connection attempt would be bound to the same (pretty low) timeout.
Do you think this is a reasonable approach? Also, is this considered a semver-major change? I think people are affected by this so we might want to include in 20.x anyway.

Fixes #47822.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2023
@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 May 4, 2023
@ShogunPanda ShogunPanda force-pushed the autoselectfamily-timeout branch from 28763f1 to 28d9306 Compare May 4, 2023 12:50
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

As far as I can tell from https://github.com/nodejs/reliability, tests such as test-x509-escaping started being flaky with ERR_SOCKET_CONNECTION_TIMEOUT from within internalConnectMultiple in the first half of April, shortly after #46790 was merged. Is this PR a fix for those flaky tests, too?

test/parallel/test-net-autoselectfamily.js Outdated Show resolved Hide resolved
@ShogunPanda
Copy link
Contributor Author

@tniessen I haven't checked specifically, but I think so.

Co-authored-by: Tobias Nießen <tniessen@tnie.de>
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ShogunPanda ShogunPanda added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 11, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 11, 2023
@nodejs-github-bot nodejs-github-bot merged commit 2d24b29 into nodejs:main May 11, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2d24b29

@ShogunPanda ShogunPanda deleted the autoselectfamily-timeout branch May 11, 2023 22:47
targos pushed a commit that referenced this pull request May 12, 2023
PR-URL: #47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos
Copy link
Member

targos commented May 15, 2023

This seems to introduce a crash in some cases: #48000

@targos
Copy link
Member

targos commented May 15, 2023

I'll exclude this change from v20.2.0 while it's being investigated.

@targos targos added the backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. label May 15, 2023
@ShogunPanda
Copy link
Contributor Author

Thanks @targos. I'll take a look soon

@dharesign
Copy link
Contributor

See my update here. I think there might be additional fixes required (moving this block to here).

@targos targos removed the backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. label Jun 4, 2023
targos pushed a commit that referenced this pull request Jun 4, 2023
PR-URL: #47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos targos mentioned this pull request Jun 4, 2023
juanarbol pushed a commit to juanarbol/node that referenced this pull request Jun 7, 2023
PR-URL: nodejs#47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Jun 21, 2023
PR-URL: nodejs#47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 6, 2023
juanarbol pushed a commit to juanarbol/node that referenced this pull request Jul 7, 2023
PR-URL: nodejs#47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Jul 13, 2023
PR-URL: nodejs#47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Jul 17, 2023
PR-URL: nodejs#47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Jul 26, 2023
PR-URL: nodejs#47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 4, 2023
PR-URL: nodejs#47860
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ruyadorno pushed a commit that referenced this pull request Aug 14, 2023
PR-URL: #47860
Backport-PR-URL: #49016
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@ruyadorno ruyadorno added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Aug 14, 2023
@ruyadorno ruyadorno mentioned this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v18.x PRs backported to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v20.0.0 ERR_SOCKET_CONNECTION_TIMEOUT when sending http request to some domains
9 participants