refactor(webtransport): maConn owns session#3498
Draft
paschal533 wants to merge 3 commits into
Draft
Conversation
… pattern The WebTransport maConn now directly owns the WebTransport session, mirroring how RTCPeerConnectionMultiaddrConnection owns RTCPeerConnection in the WebRTC transport. Before this change, the session lived in a closure inside dial() via a cleanUpWTSession callback, requiring a closed flag to coordinate five call sites that could all race to call wt.close(). The wt.closed promise handlers were also wired in dial() rather than in the class responsible for the connection lifecycle. Changes: session-to-conn.ts: - Replace cleanUpWTSession callback with webTransport: WebTransport field - Wire wt.closed.then/.catch in the constructor, mirroring WebRTC's onconnectionstatechange handler — clean close calls onTransportClosed(), error calls abort(err) - sendClose/sendReset call wt.close() directly (with try/catch) instead of delegating to a closure - sessionClosedByUs flag prevents incorrect remote_close metric increments when the local side initiates the close index.ts: - Remove WebTransportSessionCleanup type, cleanUpWTSession closure, closed flag, and the standalone wt.closed.then/.catch block - abortListener delegates to maConn.abort() when maConn exists, otherwise closes wt directly - Catch block guards against double metric-counting when the abort signal fired (timeout label already incremented by the listener) muxer.ts: - onCreateStream: catch DOMException from createBidirectionalStream() and call abort on both the muxer and maConn before rethrowing, triggering the full connection-manager cleanup chain - Incoming stream reader: same abort-on-failure pattern for the inbound path Closes libp2p#1982
3 tasks
paschal533
changed the title
paschal533
changed the title
…uxer.spec - Move WebTransport import before @libp2p/interface to satisfy import/order - Use method shorthand syntax for onSessionClose and createBidirectionalStream to satisfy @typescript-eslint/method-signature-style
tabcat
changed the title
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
1 participant
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.
Summary
Supersedes #3489 (closed). Based on feedback from @tabcat, the WebTransport transport needs a structural refactor rather than a targeted patch.
The
WebTransportSessionMultiaddrConnectionnow directly owns theWebTransportsession, mirroring howRTCPeerConnectionMultiaddrConnectionownsRTCPeerConnectionin the WebRTC transport. The old approach stored the session in a closure insidedial()and passed acleanUpWTSessioncallback to the maConn requiring aclosedflag to coordinate five call sites racing to callwt.close().Changes
session-to-conn.ts(rewritten to match WebRTC pattern):webTransport: WebTransportfield replaces thecleanUpWTSessioncallbackwt.closed.then/.catchwired in constructor, clean close →onTransportClosed(), error →abort(err), mirroring WebRTC'sonconnectionstatechangesendClose/sendResetcallwt.close()directly with try/catchsessionClosedByUsflag prevents falseremote_closemetric increments when the local side initiates the closeindex.ts(simplifieddial()):WebTransportSessionCleanuptype,cleanUpWTSessionclosure,closedflag, standalonewt.closed.then/.catchblockabortListenerdelegates tomaConn.abort()when maConn exists, otherwise closeswtdirectlymuxer.ts(error handling from #3489, retained):onCreateStream: catchDOMExceptionfromcreateBidirectionalStream(), abort both muxer and maConn before rethrowing, triggers full connection-manager cleanup chaintest/muxer.spec.ts(new, from #3489):createBidirectionalStreamthrow pathTest plan
npx aegir test -t node, 2 muxer unit tests passnpx tsc --noEmit, no errorsCloses #1982