[playwright-browser-tunnel] Fix stopAsync hanging while waiting for a connection - #5935
Conversation
… connection In poll-connection mode the init promise only settles once a client connects. stopAsync() cleared the polling interval but still awaited that promise, so stopping the tunnel before any client arrived never completed. The pending wait is now settled as part of the teardown, and a stop during the wait is treated as an ordinary shutdown by the start loop rather than an error.
| /** | ||
| * Thrown internally to settle a connection wait that was still pending when the tunnel was stopped. | ||
| */ | ||
| class TunnelStoppedError extends Error { |
There was a problem hiding this comment.
Good catch, and it did break the build exactly as described. Fixed in 086c75e: the error class now sits above the @beta block, so the tag binds to PlaywrightTunnel again and API Extractor is happy. CI went from six failing jobs to green on that commit.
| this._cancelPollConnection = (error: Error): void => { | ||
| if (this._pollInterval) { | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = undefined; | ||
| } | ||
| this._pendingConnectionAttempt = undefined; | ||
| reject(error); |
There was a problem hiding this comment.
You're right, and this is the more interesting half — thank you. My cancellation only settled the outer promise, so an attempt already in flight kept its continuation, and that continuation cleared _pollInterval, _pendingConnectionAttempt and _cancelPollConnection unconditionally. After a Stop/Start those fields belong to the new poll, so the stale continuation would wipe the new canceller and the next Stop would hang again — the exact failure this PR set out to remove. The socket it had just opened was also left with nobody to close it.
Fixed in 9650a54 with a generation counter rather than an abort signal, to keep the change small:
_pollConnectionAsynctakesconst generation = ++this._pollGenerationand derivesownsPollState()from it.- Cancelling bumps
_pollGeneration, so everything already in flight is retired at that moment. - A continuation that no longer owns the generation closes its socket (
NORMAL_CLOSURE, "Tunnel stopped") and returns without touching shared state. The.catchpath is guarded the same way, so a late failure cannot clear a live_pendingConnectionAttempt.
I did not add a test for it: the package has no test setup, and reproducing the race needs _tryConnectAsync stubbed, which is private. If you would rather see it covered, I am happy to add a small harness in this PR — just say which shape you prefer.
The new error type was declared between PlaywrightTunnel's doc comment and the class, so the @beta tag bound to the error instead and API Extractor saw PlaywrightTunnel as an undocumented public export.
…he next one Cancelling only rejected the outer promise. A _tryConnectAsync() already in flight kept running, and its continuation cleared the interval, the pending attempt and the canceller — state that by then could belong to a poll started after the stop, which would leave the next stop hanging again. It also left the socket it had just opened with no owner. Each poll now takes a generation. A continuation that no longer owns it closes its socket and touches nothing else.
Summary
Fixes #5853
PlaywrightTunnel.stopAsync()never completes when the tunnel is stopped inpoll-connectionmode before any client has connected. The tunnel is left inwaiting-for-connectionand the caller waits forever.Details
_pollConnectionAsync()returns a promise that is only resolved from inside the polling interval, when_tryConnectAsync()finally succeeds — itsrejectis never called.startAsync()stores that promise as_initWsPromise, andstopAsync()clears the interval and then awaits it. Clearing the interval stops the polling but leaves the promise pending, so the await has nothing to wait for.The fix records a canceller while the poll is in flight and settles it during teardown:
_pollConnectionAsync()registers_cancelPollConnection, which clears the poll state and rejects the pending wait; it is dropped as soon as a client connects.stopAsync()invokes it before awaiting_initWsPromise, swallows that expected rejection, clears_initWsPromise, and sets the status tostopped.startAsync()loop treats such a cancellation as an ordinary shutdown, so stopping the tunnel does not surface as a failure to whoever awaitedstartAsync().This is the case JM (@justinTM)'s #5851 explicitly does not cover — that PR fixes the Start/Stop race in the VS Code extension, while this one is in the tunnel itself, so the two do not touch the same files.
How it was tested
Against the published
@rushstack/playwright-browser-tunnel0.3.27, driving a tunnel at an endpoint where nothing is listening, so it stays inwaiting-for-connection:With this change applied to the package's compiled output, the same script:
The package currently has no unit tests, so I did not add one rather than introduce a test harness in a bug-fix PR — glad to add one if you would like it here. I also could not compile the monorepo locally, so that is worth confirming in CI.