Skip to content

Temporarily increase network_family_autoselection_attempt_timeout to 500ms - #56738

Open
hvanness wants to merge 1 commit into
nodejs:mainfrom
hvanness:network_family_autoselection_attempt_timeout_500
Open

Temporarily increase network_family_autoselection_attempt_timeout to 500ms#56738
hvanness wants to merge 1 commit into
nodejs:mainfrom
hvanness:network_family_autoselection_attempt_timeout_500

Conversation

@hvanness

@hvannesshvanness commented Jan 24, 2025

Copy link
Copy Markdown

The timeout currently closes the connection instead of supporting parallel connections. This should be reset to 250ms if parallel connections are implemented as per RFC 8305.

This PR was initiated in this discussion, where @ShogunPanda suggested 300ms - 500ms timeout.

@nodejs-github-bot

nodejs-github-bot commented Jan 24, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jan 24, 2025
@hvanness

hvanness commented Jan 24, 2025

Copy link
Copy Markdown
Author

Please add notable-change label.
These docs will need to be updated

@mcollina

mcollina commented Jan 24, 2025

Copy link
Copy Markdown
Member

Can you articulate why we should be doubling this? The other discussion is very long.

@codecov

codecovBot commented Jan 24, 2025•
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.21%. Comparing base (50d405a) to head (92e1aaa).
⚠️ Report is 3222 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #56738 +/- ##
==========================================
- Coverage 89.22% 89.21% -0.02% 
==========================================
Files 663 663 Lines 191974 192012 +38 Branches 36926 36933 +7 ==========================================
+ Hits 171286 171294 +8 - Misses 13561 13575 +14 - Partials 7127 7143 +16
Files with missing linesCoverage Δ
src/node_options.h98.33% <100.00%> (+<0.01%)⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ShogunPanda

ShogunPanda commented Jan 24, 2025

Copy link
Copy Markdown
Contributor

@hvanness Can you please update failing tests? Once you do that, this PR will LGTM.

@hvanness
hvannessforce-pushed the network_family_autoselection_attempt_timeout_500 branch from 9de2507 to f527634CompareJanuary 28, 2025 01:30
@hvanness

hvanness commented Jan 28, 2025

Copy link
Copy Markdown
Author

Can you articulate why we should be doubling this? The other discussion is very long.

This is a socket connection hard timeout, which should eventually be removed from the implementation.

For now, increasing it to 500ms resolves the majority of breaking cases:
250ms is about how long it takes to connect from Australia to US East.

PS. I maintain that parallel connections are equally safe as serial. If it is possible to stop a connection to make it serial, then it must be possible to stop all connections when one parallel connection is established.

@ShogunPanda

ShogunPanda commented Jan 28, 2025

Copy link
Copy Markdown
Contributor

PS. I maintain that parallel connections are equally safe as serial. If it is possible to stop a connection to make it serial, then it must be possible to stop all connections when one parallel connection is established.

Unfortunately that's not the case. Sockets are created at C++ layer and linked to JS objects. The code is structured to have one C++ socket per JS socket and having multiple would impact significant refactoring in the code, especially w.r.t. callbacks established in many other places of the code (like TLS).

@hvanness
hvannessforce-pushed the network_family_autoselection_attempt_timeout_500 branch from b82a398 to e26919cCompareJanuary 30, 2025 22:43
@hvanness
net: Temporarily inc network_family_autoselection_attempt_timeout to …
92e1aaa
…500ms
The timeout currently closes the connection instead of supporting
parallel connections. This should be reset to 250ms if parallel
connections are implemented as per RFC 8305.
@hvanness
hvannessforce-pushed the network_family_autoselection_attempt_timeout_500 branch from e26919c to 92e1aaaCompareJanuary 30, 2025 22:55
@MobinAskari

MobinAskari commented Apr 8, 2025

Copy link
Copy Markdown

Temporarily?
Just let it stay at 500ms, it's probably not even enough considering the actual latency the services experience.

@ShogunPanda

ShogunPanda commented Apr 8, 2025

Copy link
Copy Markdown
Contributor

My problem with this PR is that while increasing the timeout is semver-minor, decreasing would be semver-major (unless we opt to revert the change, which is not always a good idea).

I'll bring this into the next TSC to get a line of action.

@ShogunPandaShogunPanda added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 8, 2025
RafaelGSS
RafaelGSS approved these changes Apr 9, 2025
View reviewed changes

@RafaelGSSRafaelGSS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@RafaelGSSRafaelGSS removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Apr 9, 2025
@richardlaurichardlau added the baking-for-lts PRs that need to wait before landing in a LTS release. label Apr 9, 2025
BridgeAR
BridgeAR reviewed Apr 9, 2025
View reviewed changes

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The increase itself is LGTM. Thanks for the PR! We just need to document the change in the changes section, so that users know about it.

Similar to e.g., https://github.com/nodejs/node/pull/56738/files#diff-d76469e9e7f555294a7a5488c5c8fc4ef8ce5aea448cc26a1322d1ab693e09caL1713-L1724

Please also change the commit message to adhere to our commit message guidelines as outlined in the pull request contributing document :)
It should actually not be a temporary increase. We likely stick with this until we find a completely different solution.

I didn't check if anything else has to be changed here.

@richardlau

richardlau commented Apr 9, 2025

Copy link
Copy Markdown
Member

Applied the baking-for-lts labelled as discussed in today's TSC meeting. This label signifies that this should be considered for LTS versions of Node.js, but not immediately backported because we would like to see if any issues arise when this is included on a current release first.

@ShogunPanda

ShogunPanda commented Apr 19, 2025

Copy link
Copy Markdown
Contributor

@hvanness If you fix the commit we will be happy to land this.

@ivanjhivanjh mentioned this pull request Apr 28, 2025
Closed
@cgrayson

cgrayson commented Jun 30, 2025

Copy link
Copy Markdown

@hvanness did you plan to make the requested change to this commit so they could land it?

mcollina
mcollina approved these changes Jul 2, 2025
View reviewed changes

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@mcollina

mcollina commented Jul 2, 2025

Copy link
Copy Markdown
Member

Can you adjust the first commit to follow our guidelines?

@rvaggrvagg mentioned this pull request Oct 21, 2025
Merged
@github-actions

github-actionsBot commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@CrazyWolf13

CrazyWolf13 commented Jun 5, 2026

Copy link
Copy Markdown

@hvanness are there any intentions to continue this?

I'm facing similar issues and would love to see this merged, thanks!

@mrivas00mrivas00 mentioned this pull request Jun 12, 2026
Merged
4 tasks
mrivas00 added a commit to undp/carbon-footprint-program that referenced this pull request Jun 12, 2026
@mrivas00
[API] Fix: intermittent login 401 on high-latency links (Node happy-e…
b9594e3
…yeballs 250ms budget) (#381)
Node >=20 enables autoSelectFamily with a 250ms per-address connect
budget. On links whose TCP connect to the identity provider exceeds it
(VPNs, deployments far from Microsoft endpoints), every outbound
connection attempt is aborted: jwks-rsa cannot download signing keys and
the API rejects ALL valid tokens with an empty 401 — intermittently,
since real-world latency fluctuates around the threshold. The same
budget governs the http/https agents used by the storage SDKs.
Known upstream issue (nodejs/node#54359). Node 25.2 raised the default
to 500ms (nodejs/node#60334), still tight for high-RTT links and not
backported to 24 LTS, so set the 2500ms value originally proposed
upstream (nodejs/node#56738) at bootstrap. Connections that finish
faster are unaffected; only failover to dead addresses gets slower.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeARBridgeARBridgeAR left review comments
@mcollinamcollinamcollina approved these changes
@RafaelGSSRafaelGSSRafaelGSS approved these changes
@ShogunPandaShogunPandaAwaiting requested review from ShogunPanda

Assignees

No one assigned

Labels

baking-for-ltsPRs that need to wait before landing in a LTS release.c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@hvanness@nodejs-github-bot@mcollina@ShogunPanda@MobinAskari@richardlau@cgrayson@CrazyWolf13@BridgeAR@RafaelGSS