Conversation
Keep pending TCP connections alive when starting fallback attempts. The first successful connection wins; fixed local ports retain sequential attempts. Assisted-by: pi coding agent Signed-off-by: Matteo Collina <hello@matteocollina.com>
|
Review requested:
|
nodejs-github-bot
added
needs-ci
|
@nodejs/tsc PTAL. Also: how we should consider this wrt semver? I would propose a minor+”backing for LTS”, so we can land on 26 and see. If you want to push to 27.. it’s also ok. |
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
There's some notable bugs in here, and the tests are failing.
Aside from that, I think this is indeed semver-mejor. There's a fair few observable changes that are unavoidable with this approach, like:
- All access to any local binding state like
socket.address()now stops working until the connection completes - it just returns{}just like before callingconnect(). With this design, it fundamentally can never return a local address, because we're using multiple addresses in parallel until one succeeds. - The various
connectionAttempt*events now all behave very differently:connectionAttemptevents no longer pair with the others (they fire in parallel, not series) and after a timeout event you can now see a success or failure event for the same attempt. AggregateError.errorswas always in happy eyeballs order before, and matched the attempt events - now it's in arbitrary completion order.
I do still think we should do this though, as semver-major. It's a better option that might reduce lots of the timeout issues we've seen. It does change a chunk of observable behaviour en route, but I think the end result is reasonable and worth it.
| handle.close(); | ||
| ArrayPrototypePush(context.errors, new ExceptionWithHostPort(err, 'bind', localAddress, localPort)); | ||
| internalConnectMultiple(context); | ||
| scheduleConnectionAttempt(context, 10); |
There was a problem hiding this comment.
This adds a 10ms delay after explicit connection rejections.
With this PR as is, for the common case of IPv4-only localhost server (where we try IPv6, fail with ECONNRESET, then fallback to IPv4) connection time jumps from sub millisecond currently to 10ms minimum.
The RFC does suggest we should use a 10ms minimum, but all the explanation around it makes it clear that this is worrying about packet loss & timeouts, and refers to simple implementations only. Non-timeout cases should clearly bypass this imo.
Imo we should keep the old behaviour, and instantly move forward after errors.
| self.destroy(context.errors.length === 0 ? | ||
| new ERR_SOCKET_CONNECTION_TIMEOUT() : new NodeAggregateError(context.errors)); | ||
| } | ||
| return; |
There was a problem hiding this comment.
If the final candidate fails explicitly but there are still other previous requests pending, we can get stuck.
Two routes:
- A final call to scheduleConnectionAttempt/internalConnectMultiple after sync failure (e.g. blocklist) ends up here. Given preceeding requests still pending it skips the
destroy()call. - After async final failure with pending requests, in afterConnectMultiple we similarly
returnwithout any action at 2205.
If we hit either case, we just do nothing. We've cleared all the timeouts, but we don't clean up.
If the pending requests never resolve (no response at all) then in practice I think we wait here until the OS times out for us - for Linux defaults for example it looks like this will wait for a little over 2 minutes.
I think we need a final step here. Maybe wait one more timeout and then kill everything? Would be nice to bound that extra timeout tighter somehow (use the correct remaining timeout from the latest of the pending requests somehow) but probably not worth the extra complexity.
| context[kTimeout] = attempt ? | ||
| setTimeout(internalConnectMultipleTimeout, delay, context, attempt) : | ||
| setTimeout(internalConnectMultiple, delay, context); | ||
| if (context.socket._handle?.hasRef?.() === false) context[kTimeout].unref(); |
There was a problem hiding this comment.
I was surprised by the ?. after hasRef here - I think that's been added as a workaround that hides a worse bug:
All of this will also be called as part of tlssocket.connect, where _handle is a TLSWrap, not a TCPWrap, and it looks like TLSWrap doesn't currently have hasRef, so this'll breaks unref behaviour for all TLS.
I think that's just a straight TLSWrap bug, and so we should add hasRef there and then we can drop the ?. here (if the handle is set, hasRef should work).
jasnell
added
the
semver-major
Run the first family autoselection attempt on the socket's own handle, so state set on it before connecting (TLS wrapping, onread buffers, socket options) is kept. Additional attempts use new handles and replace it only if they win. Start the next attempt immediately after an explicit failure. If the last attempt fails while others are pending, give them one more attempt timeout. Report AggregateError errors in attempt order. Add hasRef() to the TLSWrap proxied methods. Signed-off-by: Matteo Collina <hello@matteocollina.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #66229 +/- ##
==========================================
+ Coverage 90.24% 90.41% +0.17%
==========================================
Files 789 791 +2
Lines 270613 275713 +5100
Branches 51802 52894 +1092
==========================================
+ Hits 244208 249292 +5084
+ Misses 16870 16822 -48
- Partials 9535 9599 +64
🚀 New features to boost your workflow:
|
mcollina
added
the
request-ci
github-actions
Bot
removed
the
request-ci
6 participants
Keep pending TCP connections alive when starting
fallback attempts. The first successful connection wins; fixed
local ports retain sequential attempts.
——
AI generated, humanly reviewed.