-
Notifications
You must be signed in to change notification settings - Fork 707
[playwright-browser-tunnel] Fix stopAsync hanging while waiting for a connection #5935
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,6 +83,15 @@ interface IBrowserServerProxy { | |
| client: WebSocket; | ||
| } | ||
|
|
||
| /** | ||
| * Thrown internally to settle a connection wait that was still pending when the tunnel was stopped. | ||
| */ | ||
| class TunnelStoppedError extends Error { | ||
| public constructor() { | ||
| super('The tunnel was stopped while waiting for a connection'); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Hosts a Playwright browser server and forwards traffic over a WebSocket tunnel. | ||
| * @beta | ||
|
|
@@ -97,6 +106,10 @@ export class PlaywrightTunnel { | |
| private readonly _playwrightInstallPath: string; | ||
| private _status: TunnelStatus = 'stopped'; | ||
| private _initWsPromise?: Promise<WebSocket>; | ||
| private _cancelPollConnection?: (error: Error) => void; | ||
| /// Bumped whenever polling starts or is cancelled, so an attempt that resolves | ||
| /// after a stop or restart can recognise that it no longer owns the shared state. | ||
| private _pollGeneration: number = 0; | ||
| private _keepRunning: boolean = false; | ||
| private _ws?: WebSocket; | ||
| private _mode: TunnelMode; | ||
|
|
@@ -163,7 +176,14 @@ export class PlaywrightTunnel { | |
| } else { | ||
| terminal.writeLine(`Tunnel is already running with status: ${this.status}`); | ||
| } | ||
| await this.waitForCloseAsync(); | ||
| try { | ||
| await this.waitForCloseAsync(); | ||
| } catch (error) { | ||
| // stopAsync() settles a pending connection wait; that is an ordinary shutdown, not a failure | ||
| if (this._keepRunning || !(error instanceof TunnelStoppedError)) { | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -173,9 +193,28 @@ export class PlaywrightTunnel { | |
| clearInterval(this._pollInterval); | ||
| this._pollInterval = undefined; | ||
| } | ||
| await this._initWsPromise?.finally(() => { | ||
| this._ws?.close(WebSocketCloseCode.NORMAL_CLOSURE, 'Tunnel stopped'); | ||
| }); | ||
| this._pendingConnectionAttempt = undefined; | ||
|
|
||
| // In poll-connection mode the init promise only settles once a client connects. Clearing the | ||
| // interval stops the polling but leaves that promise pending forever, so stopping before any | ||
| // client arrived would never complete. Settle it explicitly as part of the teardown. | ||
| const cancelPollConnection: ((error: Error) => void) | undefined = this._cancelPollConnection; | ||
| this._cancelPollConnection = undefined; | ||
| cancelPollConnection?.(new TunnelStoppedError()); | ||
|
|
||
| const initWsPromise: Promise<WebSocket> | undefined = this._initWsPromise; | ||
| this._initWsPromise = undefined; | ||
| try { | ||
| await initWsPromise?.finally(() => { | ||
| this._ws?.close(WebSocketCloseCode.NORMAL_CLOSURE, 'Tunnel stopped'); | ||
| }); | ||
| } catch (error) { | ||
| if (!(error instanceof TunnelStoppedError)) { | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| this.status = 'stopped'; | ||
| } | ||
|
|
||
| public async [Symbol.asyncDispose](): Promise<void> { | ||
|
|
@@ -271,7 +310,20 @@ export class PlaywrightTunnel { | |
| // Need to support multiple simultaneous connections for parallel tests. | ||
| private async _pollConnectionAsync(): Promise<WebSocket> { | ||
| this._terminal.writeLine(`Waiting for WebSocket connection`); | ||
| const generation: number = ++this._pollGeneration; | ||
| const ownsPollState = (): boolean => this._pollGeneration === generation; | ||
| return await new Promise((resolve, reject) => { | ||
| this._cancelPollConnection = (error: Error): void => { | ||
| // Retire this generation so an attempt still in flight cannot clear the | ||
| // interval or the canceller belonging to whatever starts next. | ||
| this._pollGeneration += 1; | ||
| if (this._pollInterval) { | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = undefined; | ||
| } | ||
| this._pendingConnectionAttempt = undefined; | ||
| reject(error); | ||
|
Comment on lines
+316
to
+325
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Fixed in 9650a54 with a generation counter rather than an abort signal, to keep the change small:
I did not add a test for it: the package has no test setup, and reproducing the race needs |
||
| }; | ||
| this._pollInterval = setInterval(() => { | ||
| if (this._pendingConnectionAttempt) { | ||
| return; // Skip if a connection attempt is already in progress | ||
|
|
@@ -280,15 +332,25 @@ export class PlaywrightTunnel { | |
| this._pendingConnectionAttempt = connectionPromise; | ||
| connectionPromise | ||
| .then((ws: WebSocket) => { | ||
| if (!ownsPollState()) { | ||
| // Stopped or restarted while this attempt was in flight: the socket | ||
| // belongs to nobody now, so close it rather than leave it open. | ||
| ws.removeAllListeners(); | ||
| ws.close(WebSocketCloseCode.NORMAL_CLOSURE, 'Tunnel stopped'); | ||
| return; | ||
| } | ||
| clearInterval(this._pollInterval); | ||
| this._pollInterval = undefined; | ||
| ws.removeAllListeners(); | ||
| this._pendingConnectionAttempt = undefined; | ||
| this._cancelPollConnection = undefined; | ||
| resolve(ws); | ||
| }) | ||
| .catch(() => { | ||
| // no-op - will retry on next interval | ||
| this._pendingConnectionAttempt = undefined; | ||
| if (ownsPollState()) { | ||
| this._pendingConnectionAttempt = undefined; | ||
| } | ||
| }); | ||
| }, 500); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "changes": [ | ||
| { | ||
| "comment": "Fix `PlaywrightTunnel.stopAsync()` hanging when the tunnel is stopped in poll-connection mode before any client has connected.", | ||
| "type": "patch", | ||
| "packageName": "@rushstack/playwright-browser-tunnel" | ||
| } | ||
| ], | ||
| "packageName": "@rushstack/playwright-browser-tunnel" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, and it did break the build exactly as described. Fixed in 086c75e: the error class now sits above the
@betablock, so the tag binds toPlaywrightTunnelagain and API Extractor is happy. CI went from six failing jobs to green on that commit.