Repository navigation
Conversation
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
|
Thanks for this, Matt. The approach holds up, and it is continuing in moq-dev/moq#6608. This branch lives on an organization-owned fork, and GitHub does not let maintainers push to those even with "Allow edits by maintainers" on. So #6608 carries your commit unchanged (your authorship kept, no rebase), plus a merge of This PR stays open for now. (Written by Claude Opus 5.5) |
Collaborator
|
Superseded by #6608, which carried this change through review and has merged. Closing as agreed with the maintainer. (Written by Claude Opus 5.5) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
connect()has no deadline, andReloadhands it only its teardown signal. So a dial the network never answers holds the reconnect loop until the browser or OS gives up.delay.timeoutis only checked after an attempt fails, so it doesn't help.This happens when a client reconnects through a layer-4 load balancer.
In Chrome 152 on macOS, against an address that never answers:
Promise.anywaits until both fail, so an attempt whose dials all land on the dead target waits 75 s. Where WebSocket is the only transport (Safari, Firefox before 153, Node, Bun, Deno), one dead dial is enough. A browser page in this situation took 80 s to reconnect.Approach
Four parts, all in
connect(), soReload, the pool's shared and private loops, GOAWAY migration and a one-shotconnect()all get them.ConnectProps.timeoutandConnectionProps.timeout(default 30 s;Milli(0)for none) cover the dial and the MoQ handshake.Error.ConnectTimeout, whichReloadretries like any other failure.connect.timeoutandError::ConnectTimeout(fix(native): bound Client::connect so a silent peer can't wedge reconnect #2622). The pairtimeout/delay.timeoutmirrorsconnect.timeout/connect.backoff.timeout.allowPooling: false), so each dial is a new flow.no transport connected: …keeps one reason per transport. A browser words every handshake failure the same way.WebSocketStreamandWebSocket:WebSocket), a second WebSocket dialed 1.8 s in failed 75.0 s after the first was closed (at 5.8 s), not 75 s after its own dial. So it had been queued too.Consumers:
<moq-watch>and<moq-publish>use the shared pool, so they get all of this through the defaults. Thetimeoutoptions are the escape hatch described in MIGRATE.md.Impact
ConnectProps.timeoutandConnectionProps.timeout.share: truerefusestimeout, as it doesdelay.Error.ConnectTimeout, withtimeoutin milliseconds.connect()and eachConnectionattempt reject withError.ConnectTimeoutafter 30 s. A deadline set throughsignalthat is longer than that no longer takes effect.timeout(negative, NaN, infinite) is aRangeError:connect()rejects, and aConnectionthrows when it is built.Alternatives
connect()calls insideReload: a near-tie wires both sessions into the origins and announces on both, an auth rejection gets lost in theAggregateError, and the WebSocket gains nothing because of the per-address limit.websocketWonis: it would halve the throttle cost in a long outage, but slow recovery when both of an attempt's dials happen to land on dead targets.Follow-ups
accept()has no SETUP deadline on the server side either. That's a different problem (what a silent peer can make a server hold); happy to open a quest.Decisions for you, with my recommendation
ConnectPropsnext totimeout.(Written by Claude Opus 5.5)