fix(connect): handle disconnect in various situations (#6276)

There are a few ways for `connect()` to finish:
- `Browser.close()` from the client side.
- Browser on the server side did exit (e.g. crashed).
- Connection was dropped by either of the sides.

We reduce all the cases to the last one by dropping the
connection when client wants calls `Browser.close()` or
server-side browser exits.

In all these cases we should properly cleanup on the server side,
and ensure that all promises reject on the client side.
This commit is contained in:
Dmitry Gozman 2021-05-06 09:34:06 -07:00 committed by GitHub
parent d902b06fd1
commit b29b7df47e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
16 changed files with 185 additions and 57 deletions

View file

@ -18,8 +18,7 @@ import { LaunchServerOptions, Logger } from './client/types';
import { BrowserType } from './server/browserType';
import { Browser } from './server/browser';
import { EventEmitter } from 'ws';
import { DispatcherScope } from './dispatchers/dispatcher';
import { BrowserDispatcher } from './dispatchers/browserDispatcher';
import { Dispatcher, DispatcherScope } from './dispatchers/dispatcher';
import { BrowserContextDispatcher } from './dispatchers/browserContextDispatcher';
import * as channels from './protocol/channels';
import { BrowserServerLauncher, BrowserServer } from './client/browserType';
@ -32,6 +31,10 @@ import { CallMetadata, internalCallMetadata } from './server/instrumentation';
import { Playwright } from './server/playwright';
import { PlaywrightDispatcher } from './dispatchers/playwrightDispatcher';
import { PlaywrightServer, PlaywrightServerDelegate } from './remote/playwrightServer';
import { BrowserContext } from './server/browserContext';
import { CRBrowser } from './server/chromium/crBrowser';
import { CDPSessionDispatcher } from './dispatchers/cdpSessionDispatcher';
import { PageDispatcher } from './dispatchers/pageDispatcher';
export class BrowserServerLauncherImpl implements BrowserServerLauncher {
private _playwright: Playwright;
@ -67,6 +70,7 @@ export class BrowserServerLauncherImpl implements BrowserServerLauncher {
browserServer.wsEndpoint = () => wsEndpoint;
browserServer.close = () => browser.options.browserProcess.close();
browserServer.kill = () => browser.options.browserProcess.kill();
(browserServer as any)._disconnectForTest = () => server.close();
browser.options.browserProcess.onclose = async (exitCode, signal) => {
server.close();
browserServer.emit('close', exitCode, signal);
@ -74,55 +78,78 @@ export class BrowserServerLauncherImpl implements BrowserServerLauncher {
return browserServer;
}
private _onConnect(browser: Browser, scope: DispatcherScope) {
private _onConnect(browser: Browser, scope: DispatcherScope, forceDisconnect: () => void) {
const selectors = new Selectors();
const selectorsDispatcher = new SelectorsDispatcher(scope, selectors);
const browserDispatcher = new ConnectedBrowser(scope, browser, selectors);
const browserDispatcher = new ConnectedBrowserDispatcher(scope, browser, selectors);
browser.on(Browser.Events.Disconnected, () => {
// Underlying browser did close for some reason - force disconnect the client.
forceDisconnect();
});
new PlaywrightDispatcher(scope, this._playwright, selectorsDispatcher, browserDispatcher);
return () => {
// Cleanup contexts upon disconnect.
browserDispatcher.close().catch(e => {});
browserDispatcher.cleanupContexts().catch(e => {});
};
}
}
// This class implements multiplexing multiple BrowserDispatchers over a single Browser instance.
class ConnectedBrowser extends BrowserDispatcher {
private _contexts: BrowserContextDispatcher[] = [];
// This class implements multiplexing browser dispatchers over a single Browser instance.
class ConnectedBrowserDispatcher extends Dispatcher<Browser, channels.BrowserInitializer> implements channels.BrowserChannel {
private _contexts = new Set<BrowserContext>();
private _selectors: Selectors;
private _closed = false;
constructor(scope: DispatcherScope, browser: Browser, selectors: Selectors) {
super(scope, browser);
super(scope, browser, 'Browser', { version: browser.version(), name: browser.options.name }, true);
this._selectors = selectors;
}
async newContext(params: channels.BrowserNewContextParams, metadata: CallMetadata): Promise<{ context: channels.BrowserContextChannel }> {
async newContext(params: channels.BrowserNewContextParams, metadata: CallMetadata): Promise<channels.BrowserNewContextResult> {
if (params.recordVideo) {
// TODO: we should create a separate temp directory or accept a launchServer parameter.
params.recordVideo.dir = this._object.options.downloadsPath!;
}
const result = await super.newContext(params, metadata);
const dispatcher = result.context as BrowserContextDispatcher;
dispatcher._object._setSelectors(this._selectors);
this._contexts.push(dispatcher);
return result;
const context = await this._object.newContext(params);
this._contexts.add(context);
context._setSelectors(this._selectors);
context.on(BrowserContext.Events.Close, () => this._contexts.delete(context));
if (params.storageState)
await context.setStorageState(metadata, params.storageState);
return { context: new BrowserContextDispatcher(this._scope, context) };
}
async close(): Promise<void> {
// Only close our own contexts.
await Promise.all(this._contexts.map(context => context.close({}, internalCallMetadata())));
this._didClose();
// Client should not send us Browser.close.
}
_didClose() {
if (!this._closed) {
// We come here multiple times:
// - from ConnectedBrowser.close();
// - from underlying Browser.on('close').
this._closed = true;
super._didClose();
}
async killForTests(): Promise<void> {
// Client should not send us Browser.killForTests.
}
async newBrowserCDPSession(): Promise<channels.BrowserNewBrowserCDPSessionResult> {
if (!this._object.options.isChromium)
throw new Error(`CDP session is only available in Chromium`);
const crBrowser = this._object as CRBrowser;
return { session: new CDPSessionDispatcher(this._scope, await crBrowser.newBrowserCDPSession()) };
}
async startTracing(params: channels.BrowserStartTracingParams): Promise<void> {
if (!this._object.options.isChromium)
throw new Error(`Tracing is only available in Chromium`);
const crBrowser = this._object as CRBrowser;
await crBrowser.startTracing(params.page ? (params.page as PageDispatcher)._object : undefined, params);
}
async stopTracing(): Promise<channels.BrowserStopTracingResult> {
if (!this._object.options.isChromium)
throw new Error(`Tracing is only available in Chromium`);
const crBrowser = this._object as CRBrowser;
const buffer = await crBrowser.stopTracing();
return { binary: buffer.toString('base64') };
}
async cleanupContexts() {
await Promise.all(Array.from(this._contexts).map(context => context.close(internalCallMetadata())));
}
}

View file

@ -28,7 +28,7 @@ export class Browser extends ChannelOwner<channels.BrowserChannel, channels.Brow
readonly _contexts = new Set<BrowserContext>();
private _isConnected = true;
private _closedPromise: Promise<void>;
_isRemote = false;
_remoteType: 'owns-connection' | 'uses-connection' | null = null;
readonly _name: string;
static from(browser: channels.BrowserChannel): Browser {
@ -98,7 +98,10 @@ export class Browser extends ChannelOwner<channels.BrowserChannel, channels.Brow
async close(): Promise<void> {
try {
await this._wrapApiCall('browser.close', async (channel: channels.BrowserChannel) => {
await channel.close();
if (this._remoteType === 'owns-connection')
this._connection.close();
else
await channel.close();
await this._closedPromise;
});
} catch (e) {

View file

@ -21,7 +21,6 @@ import { ChannelOwner } from './channelOwner';
import { LaunchOptions, LaunchServerOptions, ConnectOptions, LaunchPersistentContextOptions } from './types';
import WebSocket from 'ws';
import { Connection } from './connection';
import { serializeError } from '../protocol/serializers';
import { Events } from './events';
import { TimeoutSettings } from '../utils/timeoutSettings';
import { ChildProcess } from 'child_process';
@ -111,32 +110,32 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
async connect(params: ConnectOptions): Promise<Browser> {
const logger = params.logger;
return this._wrapApiCall('browserType.connect', async () => {
const connection = new Connection();
const ws = new WebSocket(params.wsEndpoint, [], {
perMessageDeflate: false,
maxPayload: 256 * 1024 * 1024, // 256Mb,
handshakeTimeout: this._timeoutSettings.timeout(params),
headers: params.headers,
});
const connection = new Connection(() => ws.close());
// The 'ws' module in node sometimes sends us multiple messages in a single task.
const waitForNextTask = params.slowMo
? (cb: () => any) => setTimeout(cb, params.slowMo)
: makeWaitForNextTask();
connection.onmessage = message => {
if (ws.readyState !== WebSocket.OPEN) {
setTimeout(() => {
connection.dispatch({ id: (message as any).id, error: serializeError(new Error(kBrowserClosedError)) });
}, 0);
// Connection should handle all outgoing message in disconnected().
if (ws.readyState !== WebSocket.OPEN)
return;
}
ws.send(JSON.stringify(message));
};
ws.addEventListener('message', event => {
waitForNextTask(() => {
try {
connection.dispatch(JSON.parse(event.data));
// Since we may slow down the messages, but disconnect
// synchronously, we might come here with a message
// after disconnect.
if (!connection.isDisconnected())
connection.dispatch(JSON.parse(event.data));
} catch (e) {
ws.close();
}
@ -165,7 +164,7 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
const browser = Browser.from(playwright._initializer.preLaunchedBrowser!);
browser._logger = logger;
browser._isRemote = true;
browser._remoteType = 'owns-connection';
const closeListener = () => {
// Emulate all pages, contexts and the browser closing upon disconnect.
for (const context of browser.contexts()) {
@ -174,6 +173,7 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
context._onClose();
}
browser._didClose();
connection.didDisconnect(kBrowserClosedError);
};
ws.removeEventListener('close', prematureCloseListener);
ws.addEventListener('close', closeListener);
@ -210,7 +210,7 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
const browser = Browser.from(result.browser);
if (result.defaultContext)
browser._contexts.add(BrowserContext.from(result.defaultContext));
browser._isRemote = true;
browser._remoteType = 'uses-connection';
browser._logger = logger;
return browser;
}, logger);

View file

@ -23,7 +23,7 @@ import type { Connection } from './connection';
import type { Logger } from './types';
export abstract class ChannelOwner<T extends channels.Channel = channels.Channel, Initializer = {}> extends EventEmitter {
private _connection: Connection;
protected _connection: Connection;
private _parent: ChannelOwner | undefined;
private _objects = new Map<string, ChannelOwner>();

View file

@ -52,9 +52,12 @@ export class Connection {
private _lastId = 0;
private _callbacks = new Map<number, { resolve: (a: any) => void, reject: (a: Error) => void }>();
private _rootObject: ChannelOwner;
private _disconnectedErrorMessage: string | undefined;
private _onClose?: () => void;
constructor() {
constructor(onClose?: () => void) {
this._rootObject = new Root(this);
this._onClose = onClose;
}
async waitForObjectWithKnownName(guid: string): Promise<any> {
@ -75,6 +78,8 @@ export class Connection {
debugLogger.log('channel:command', converted);
this.onmessage({ ...converted, metadata: { stack: frames, apiName } });
try {
if (this._disconnectedErrorMessage)
throw new Error(this._disconnectedErrorMessage);
return await new Promise((resolve, reject) => this._callbacks.set(id, { resolve, reject }));
} catch (e) {
const innerStack = ((process.env.PWDEBUGIMPL || isUnderTest()) && e.stack) ? e.stack.substring(e.stack.indexOf(e.message) + e.message.length) : '';
@ -120,6 +125,22 @@ export class Connection {
object._channel.emit(method, this._replaceGuidsWithChannels(params));
}
close() {
if (this._onClose)
this._onClose();
}
didDisconnect(errorMessage: string) {
this._disconnectedErrorMessage = errorMessage;
for (const callback of this._callbacks.values())
callback.reject(new Error(errorMessage));
this._callbacks.clear();
}
isDisconnected() {
return !!this._disconnectedErrorMessage;
}
private _replaceGuidsWithChannels(payload: any): any {
if (!payload)
return payload;

View file

@ -102,6 +102,8 @@ export class Frame extends ChannelOwner<channels.FrameChannel, channels.FrameIni
private _setupNavigationWaiter(name: string, options: { timeout?: number }): Waiter {
const waiter = new Waiter(this, name);
if (this._page!.isClosed())
waiter.rejectImmediately(new Error('Navigation failed because page was closed!'));
waiter.rejectOnEvent(this._page!, Events.Page.Close, new Error('Navigation failed because page was closed!'));
waiter.rejectOnEvent(this._page!, Events.Page.Crash, new Error('Navigation failed because page crashed!'));
waiter.rejectOnEvent<Frame>(this._page!, Events.Page.FrameDetached, new Error('Navigating frame was detached!'), frame => frame === this);

View file

@ -124,7 +124,7 @@ export class Page extends ChannelOwner<channels.PageChannel, channels.PageInitia
this._channel.on('domcontentloaded', () => this.emit(Events.Page.DOMContentLoaded, this));
this._channel.on('download', ({ url, suggestedFilename, artifact }) => {
const artifactObject = Artifact.from(artifact);
artifactObject._isRemote = !!this._browserContext._browser && this._browserContext._browser._isRemote;
artifactObject._isRemote = !!this._browserContext._browser && !!this._browserContext._browser._remoteType;
this.emit(Events.Page.Download, new Download(url, suggestedFilename, artifactObject));
});
this._channel.on('fileChooser', ({ element, isMultiple }) => this.emit(Events.Page.FileChooser, new FileChooser(this, ElementHandle.from(element), isMultiple)));

View file

@ -42,7 +42,7 @@ export class Tracing {
return await channel.tracingExport();
});
const artifact = Artifact.from(result.artifact);
if (this._context.browser()?._isRemote)
if (this._context.browser()?._remoteType)
artifact._isRemote = true;
await artifact.saveAs(path);
await artifact.delete();

View file

@ -25,7 +25,7 @@ export class Video implements api.Video {
constructor(page: Page) {
const browser = page.context()._browser;
this._isRemote = !!browser && browser._isRemote;
this._isRemote = !!browser && !!browser._remoteType;
this._artifact = Promise.race([
new Promise<Artifact>(f => this._artifactCallback = f),
page._closedOrCrashedPromise.then(() => null),

View file

@ -23,6 +23,7 @@ import { ChannelOwner } from './channelOwner';
export class Waiter {
private _dispose: (() => void)[];
private _failures: Promise<any>[] = [];
private _immediateError?: Error;
// TODO: can/should we move these logs into wrapApiCall?
private _logs: string[] = [];
private _channelOwner: ChannelOwner;
@ -59,6 +60,10 @@ export class Waiter {
this._rejectOn(promise.then(() => { throw new TimeoutError(message); }), dispose);
}
rejectImmediately(error: Error) {
this._immediateError = error;
}
dispose() {
for (const dispose of this._dispose)
dispose();
@ -66,6 +71,8 @@ export class Waiter {
async waitForPromise<T>(promise: Promise<T>, dispose?: () => void): Promise<T> {
try {
if (this._immediateError)
throw this._immediateError;
const result = await Promise.race([promise, ...this._failures]);
if (dispose)
dispose();

View file

@ -21,11 +21,10 @@ import { BrowserTypeDispatcher } from './browserTypeDispatcher';
import { Dispatcher, DispatcherScope } from './dispatcher';
import { ElectronDispatcher } from './electronDispatcher';
import { SelectorsDispatcher } from './selectorsDispatcher';
import type { BrowserDispatcher } from './browserDispatcher';
import * as types from '../server/types';
export class PlaywrightDispatcher extends Dispatcher<Playwright, channels.PlaywrightInitializer> implements channels.PlaywrightChannel {
constructor(scope: DispatcherScope, playwright: Playwright, customSelectors?: SelectorsDispatcher, preLaunchedBrowser?: BrowserDispatcher) {
constructor(scope: DispatcherScope, playwright: Playwright, customSelectors?: channels.SelectorsChannel, preLaunchedBrowser?: channels.BrowserChannel) {
const descriptors = require('../server/deviceDescriptors') as types.Devices;
const deviceDescriptors = Object.entries(descriptors)
.map(([name, descriptor]) => ({ name, descriptor }));

View file

@ -28,7 +28,7 @@ const debugLog = debug('pw:server');
export interface PlaywrightServerDelegate {
path: string;
allowMultipleClients: boolean;
onConnect(rootScope: DispatcherScope): () => any;
onConnect(rootScope: DispatcherScope, forceDisconnect: () => void): () => any;
onClose: () => any;
}
@ -94,9 +94,10 @@ export class PlaywrightServer {
connection.dispatch(JSON.parse(Buffer.from(message).toString()));
});
const forceDisconnect = () => socket.close();
const scope = connection.rootDispatcher();
const onDisconnect = this._delegate.onConnect(scope);
const disconnect = () => {
const onDisconnect = this._delegate.onConnect(scope, forceDisconnect);
const disconnected = () => {
this._clientsCount--;
// Avoid sending any more messages over closed socket.
connection.onmessage = () => {};
@ -104,11 +105,11 @@ export class PlaywrightServer {
};
socket.on('close', () => {
debugLog('Client closed');
disconnect();
disconnected();
});
socket.on('error', error => {
debugLog('Client error ' + error);
disconnect();
disconnected();
});
});
@ -122,6 +123,7 @@ export class PlaywrightServer {
// First disconnect all remaining clients.
await new Promise(f => this._wsServer!.close(f));
await new Promise(f => this._wsServer!.options.server!.close(f));
this._wsServer = undefined;
await this._delegate.onClose();
}
}

View file

@ -79,7 +79,10 @@ test('disconnected event should be emitted when browser is closed or server is c
const remoteServer = await startRemoteServer();
const browser1 = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() });
await browser1.newPage();
const browser2 = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() });
await browser2.newPage();
let disconnected1 = 0;
let disconnected2 = 0;
@ -141,6 +144,19 @@ test('should throw when used after isConnected returns false', async ({browserTy
expect(error.message).toContain('has been closed');
});
test('should throw when calling waitForNavigation after disconnect', async ({browserType, startRemoteServer}) => {
const remoteServer = await startRemoteServer();
const browser = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() });
const page = await browser.newPage();
await Promise.all([
remoteServer.close(),
new Promise(f => browser.once('disconnected', f)),
]);
expect(browser.isConnected()).toBe(false);
const error = await page.waitForNavigation().catch(e => e);
expect(error.message).toContain('Navigation failed because page was closed');
});
test('should reject navigation when browser closes', async ({browserType, startRemoteServer, server}) => {
const remoteServer = await startRemoteServer();
server.setRoute('/one-style.css', () => {});
@ -150,7 +166,7 @@ test('should reject navigation when browser closes', async ({browserType, startR
await server.waitForRequest('/one-style.css');
await browser.close();
const error = await navigationPromise;
expect(error.message).toContain('Navigation failed because page was closed!');
expect(error.message).toContain('has been closed');
});
test('should reject waitForSelector when browser closes', async ({browserType, startRemoteServer, server}) => {
@ -165,7 +181,7 @@ test('should reject waitForSelector when browser closes', async ({browserType, s
await browser.close();
const error = await watchdog;
expect(error.message).toContain('Protocol error');
expect(error.message).toContain('has been closed');
});
test('should emit close events on pages and contexts', async ({browserType, startRemoteServer}) => {
@ -360,3 +376,45 @@ test('should work with cluster', async ({browserType, startRemoteServer}) => {
const page = await browser.newPage();
expect(await page.evaluate('1 + 2')).toBe(3);
});
test('should properly disconnect when connection closes from the client side', async ({browserType, startRemoteServer, server}) => {
server.setRoute('/one-style.css', () => {});
const remoteServer = await startRemoteServer();
const browser = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() });
const page = await browser.newPage();
const navigationPromise = page.goto(server.PREFIX + '/one-style.html', {timeout: 60000}).catch(e => e);
const waitForNavigationPromise = page.waitForNavigation().catch(e => e);
const disconnectedPromise = new Promise(f => browser.once('disconnected', f));
// This closes the websocket.
(browser as any)._connection.close();
await disconnectedPromise;
expect(browser.isConnected()).toBe(false);
expect((await navigationPromise).message).toContain('has been closed');
expect((await waitForNavigationPromise).message).toContain('Navigation failed because page was closed');
expect((await page.goto(server.EMPTY_PAGE).catch(e => e)).message).toContain('has been closed');
expect((await page.waitForNavigation().catch(e => e)).message).toContain('Navigation failed because page was closed');
});
test('should properly disconnect when connection closes from the server side', async ({browserType, startRemoteServer, server, platform}) => {
test.skip(platform === 'win32', 'Cannot send signals');
server.setRoute('/one-style.css', () => {});
const remoteServer = await startRemoteServer({ disconnectOnSIGHUP: true });
const browser = await browserType.connect({ wsEndpoint: remoteServer.wsEndpoint() });
const page = await browser.newPage();
const navigationPromise = page.goto(server.PREFIX + '/one-style.html', {timeout: 60000}).catch(e => e);
const waitForNavigationPromise = page.waitForNavigation().catch(e => e);
const disconnectedPromise = new Promise(f => browser.once('disconnected', f));
// This closes the websocket server.
process.kill(remoteServer.child().pid, 'SIGHUP');
await disconnectedPromise;
expect(browser.isConnected()).toBe(false);
expect((await navigationPromise).message).toContain('has been closed');
expect((await waitForNavigationPromise).message).toContain('Navigation failed because page was closed');
expect((await page.goto(server.EMPTY_PAGE).catch(e => e)).message).toContain('has been closed');
expect((await page.waitForNavigation().catch(e => e)).message).toContain('Navigation failed because page was closed');
});

View file

@ -1,7 +1,7 @@
const cluster = require('cluster');
async function start() {
const { playwrightPath, browserTypeName, launchOptions, stallOnClose } = JSON.parse(process.argv[2]);
const { playwrightPath, browserTypeName, launchOptions, stallOnClose, disconnectOnSIGHUP } = JSON.parse(process.argv[2]);
if (stallOnClose) {
launchOptions.__testHookGracefullyClose = () => {
console.log(`(stalled=>true)`);
@ -11,7 +11,12 @@ async function start() {
const playwright = require(require('path').join(playwrightPath, 'index'));
if (disconnectOnSIGHUP)
launchOptions.handleSIGHUP = false;
const browserServer = await playwright[browserTypeName].launchServer(launchOptions);
if (disconnectOnSIGHUP)
process.on('SIGHUP', () => browserServer._disconnectForTest());
browserServer.on('close', (exitCode, signal) => {
console.log(`(exitCode=>${exitCode})`);
console.log(`(signal=>${signal})`);
@ -20,6 +25,9 @@ async function start() {
console.log(`(wsEndpoint=>${browserServer.wsEndpoint()})`);
}
process.on('uncaughtException', error => console.log(error));
process.on('unhandledRejection', reason => console.log(reason));
if (cluster.isWorker || !JSON.parse(process.argv[2]).inCluster) {
start();
} else {

View file

@ -22,6 +22,7 @@ const playwrightPath = path.join(__dirname, '..', '..');
export type RemoteServerOptions = {
stallOnClose?: boolean;
disconnectOnSIGHUP?: boolean;
inCluster?: boolean;
url?: string;
};

View file

@ -24,9 +24,9 @@ it('should work', async ({page, server}) => {
});
it('should respect timeout', async ({page, server}) => {
const promise = page.waitForURL('**/frame.html', { timeout: 2500 });
const promise = page.waitForURL('**/frame.html', { timeout: 2500 }).catch(e => e);
await page.goto(server.EMPTY_PAGE);
const error = await promise.catch(e => e);
const error = await promise;
expect(error.message).toContain('page.waitForNavigation: Timeout 2500ms exceeded.');
});