-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
Temporarily increase network_family_autoselection_attempt_timeout to 500ms #56738
base: main
Are you sure you want to change the base?
Temporarily increase network_family_autoselection_attempt_timeout to 500ms #56738
Conversation
Review requested:
|
Please add notable-change label. |
Can you articulate why we should be doubling this? The other discussion is very long. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
|
@hvanness Can you please update failing tests? Once you do that, this PR will LGTM. |
9de2507
to
f527634
Compare
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: 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). |
b82a398
to
e26919c
Compare
…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.
e26919c
to
92e1aaa
Compare
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.