From a2bee2ca7396309d7800addc086f2bf66f6a28ab Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 11 May 2020 15:00:13 -0700 Subject: [PATCH] fix(launch): handle timeout and exceptions during launch (#2185) --- src/dom.ts | 2 +- src/helper.ts | 8 ++------ src/server/browserServer.ts | 19 +++++++++++++++++++ src/server/chromium.ts | 30 ++++++++++++++++++++---------- src/server/firefox.ts | 36 +++++++++++++++++++++++------------- src/server/pipeTransport.ts | 10 +++++++--- src/server/webkit.ts | 33 ++++++++++++++++++++++----------- test/launcher.spec.js | 26 ++++++++++++++++++++++++++ 8 files changed, 120 insertions(+), 44 deletions(-) diff --git a/src/dom.ts b/src/dom.ts index 31c13c7e32..576ff82848 100644 --- a/src/dom.ts +++ b/src/dom.ts @@ -470,7 +470,7 @@ export class ElementHandle extends js.JSHandle { return injected.waitForDisplayedAtStablePosition(node, rafCount, timeout); }, { rafCount, timeout: helper.timeUntilDeadline(deadline) }); const timeoutMessage = 'element to be displayed and not moving'; - const injectedResult = await helper.waitWithDeadline(stablePromise, timeoutMessage, deadline); + const injectedResult = await helper.waitWithDeadline(stablePromise, timeoutMessage, deadline, 'pw:input'); handleInjectedResult(injectedResult, timeoutMessage); this._page._log(inputLog, '...element is displayed and does not move'); } diff --git a/src/helper.ts b/src/helper.ts index b1dbca4fab..60cecab91e 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -158,13 +158,9 @@ class Helper { }); } - static async waitWithTimeout(promise: Promise, taskName: string, timeout: number): Promise { - return this.waitWithDeadline(promise, taskName, helper.monotonicTime() + timeout); - } - - static async waitWithDeadline(promise: Promise, taskName: string, deadline: number): Promise { + static async waitWithDeadline(promise: Promise, taskName: string, deadline: number, debugName: string): Promise { let reject: (error: Error) => void; - const timeoutError = new TimeoutError(`Waiting for ${taskName} failed: timeout exceeded. Re-run with the DEBUG=pw:input env variable to see the debug log.`); + const timeoutError = new TimeoutError(`Waiting for ${taskName} failed: timeout exceeded. Re-run with the DEBUG=${debugName} env variable to see the debug log.`); const timeoutPromise = new Promise((resolve, x) => reject = x); const timeoutTimer = setTimeout(() => reject(timeoutError), helper.timeUntilDeadline(deadline)); try { diff --git a/src/server/browserServer.ts b/src/server/browserServer.ts index 1de4064693..71ca39c6df 100644 --- a/src/server/browserServer.ts +++ b/src/server/browserServer.ts @@ -16,6 +16,7 @@ import { ChildProcess, execSync } from 'child_process'; import { EventEmitter } from 'events'; +import { helper } from '../helper'; export class WebSocketWrapper { readonly wsEndpoint: string; @@ -87,4 +88,22 @@ export class BrowserServer extends EventEmitter { if (this._webSocketWrapper) await this._webSocketWrapper.checkLeaks(); } + + async _initializeOrClose(deadline: number, init: () => Promise): Promise { + try { + const result = await helper.waitWithDeadline(init(), 'the browser to launch', deadline, 'pw:browser*'); + return result; + } catch (e) { + await this._closeOrKill(deadline); + throw e; + } + } + + async _closeOrKill(deadline: number): Promise { + try { + await helper.waitWithDeadline(this.close(), '', deadline, ''); // The error message is ignored. + } catch (ignored) { + this.kill(); + } + } } diff --git a/src/server/chromium.ts b/src/server/chromium.ts index ac808b4b6f..909443b32c 100644 --- a/src/server/chromium.ts +++ b/src/server/chromium.ts @@ -42,11 +42,17 @@ export class Chromium extends AbstractBrowserType { async launch(options: LaunchOptions = {}): Promise { assert(!(options as any).userDataDir, 'userDataDir option is not supported in `browserType.launch`. Use `browserType.launchPersistentContext` instead'); + const { timeout = 30000 } = options; + const deadline = TimeoutSettings.computeDeadline(timeout); const { browserServer, transport, downloadsPath, logger } = await this._launchServer(options, 'local'); - const browser = await CRBrowser.connect(transport!, false, logger, options); - browser._ownedServer = browserServer; - browser._downloadsPath = downloadsPath; - return browser; + return await browserServer._initializeOrClose(deadline, async () => { + if ((options as any).__testHookBeforeCreateBrowser) + await (options as any).__testHookBeforeCreateBrowser(); + const browser = await CRBrowser.connect(transport!, false, logger, options); + browser._ownedServer = browserServer; + browser._downloadsPath = downloadsPath; + return browser; + }); } async launchServer(options: LaunchServerOptions = {}): Promise { @@ -57,12 +63,16 @@ export class Chromium extends AbstractBrowserType { const { timeout = 30000 } = options; const deadline = TimeoutSettings.computeDeadline(timeout); const { transport, browserServer, logger } = await this._launchServer(options, 'persistent', userDataDir); - const browser = await CRBrowser.connect(transport!, true, logger, options); - browser._ownedServer = browserServer; - const context = browser._defaultContext!; - if (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs)) - await helper.waitWithTimeout(context._loadDefaultContext(), 'first page', helper.timeUntilDeadline(deadline)); - return context; + return await browserServer._initializeOrClose(deadline, async () => { + if ((options as any).__testHookBeforeCreateBrowser) + await (options as any).__testHookBeforeCreateBrowser(); + const browser = await CRBrowser.connect(transport!, true, logger, options); + browser._ownedServer = browserServer; + const context = browser._defaultContext!; + if (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs)) + await context._loadDefaultContext(); + return context; + }); } private async _launchServer(options: LaunchServerOptions, launchType: LaunchType, userDataDir?: string): Promise<{ browserServer: BrowserServer, transport?: ConnectionTransport, downloadsPath: string, logger: InnerLogger }> { diff --git a/src/server/firefox.ts b/src/server/firefox.ts index 883dd61e67..bac23e3ae6 100644 --- a/src/server/firefox.ts +++ b/src/server/firefox.ts @@ -44,13 +44,19 @@ export class Firefox extends AbstractBrowserType { async launch(options: LaunchOptions = {}): Promise { assert(!(options as any).userDataDir, 'userDataDir option is not supported in `browserType.launch`. Use `browserType.launchPersistentContext` instead'); + const { timeout = 30000 } = options; + const deadline = TimeoutSettings.computeDeadline(timeout); const { browserServer, downloadsPath, logger } = await this._launchServer(options, 'local'); - const browser = await WebSocketTransport.connect(browserServer.wsEndpoint()!, transport => { - return FFBrowser.connect(transport, logger, false, options.slowMo); + return await browserServer._initializeOrClose(deadline, async () => { + if ((options as any).__testHookBeforeCreateBrowser) + await (options as any).__testHookBeforeCreateBrowser(); + const browser = await WebSocketTransport.connect(browserServer.wsEndpoint()!, transport => { + return FFBrowser.connect(transport, logger, false, options.slowMo); + }); + browser._ownedServer = browserServer; + browser._downloadsPath = downloadsPath; + return browser; }); - browser._ownedServer = browserServer; - browser._downloadsPath = downloadsPath; - return browser; } async launchServer(options: LaunchServerOptions = {}): Promise { @@ -64,15 +70,19 @@ export class Firefox extends AbstractBrowserType { } = options; const deadline = TimeoutSettings.computeDeadline(timeout); const { browserServer, downloadsPath, logger } = await this._launchServer(options, 'persistent', userDataDir); - const browser = await WebSocketTransport.connect(browserServer.wsEndpoint()!, transport => { - return FFBrowser.connect(transport, logger, true, slowMo); + return await browserServer._initializeOrClose(deadline, async () => { + if ((options as any).__testHookBeforeCreateBrowser) + await (options as any).__testHookBeforeCreateBrowser(); + const browser = await WebSocketTransport.connect(browserServer.wsEndpoint()!, transport => { + return FFBrowser.connect(transport, logger, true, slowMo); + }); + browser._ownedServer = browserServer; + browser._downloadsPath = downloadsPath; + const context = browser._defaultContext!; + if (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs)) + await context._loadDefaultContext(); + return context; }); - browser._ownedServer = browserServer; - browser._downloadsPath = downloadsPath; - const context = browser._defaultContext!; - if (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs)) - await helper.waitWithTimeout(context._loadDefaultContext(), 'first page', helper.timeUntilDeadline(deadline)); - return context; } private async _launchServer(options: LaunchServerOptions, launchType: LaunchType, userDataDir?: string): Promise<{ browserServer: BrowserServer, downloadsPath: string, logger: InnerLogger }> { diff --git a/src/server/pipeTransport.ts b/src/server/pipeTransport.ts index 1b850afc12..3d875b3be5 100644 --- a/src/server/pipeTransport.ts +++ b/src/server/pipeTransport.ts @@ -20,10 +20,11 @@ import { ConnectionTransport, ProtocolRequest, ProtocolResponse } from '../trans import { logError, InnerLogger } from '../logger'; export class PipeTransport implements ConnectionTransport { - private _pipeWrite: NodeJS.WritableStream | null; + private _pipeWrite: NodeJS.WritableStream; private _pendingMessage = ''; private _eventListeners: RegisteredListener[]; private _waitForNextTask = helper.makeWaitForNextTask(); + private _closed = false; onmessage?: (message: ProtocolResponse) => void; onclose?: () => void; @@ -33,6 +34,7 @@ export class PipeTransport implements ConnectionTransport { this._eventListeners = [ helper.addEventListener(pipeRead, 'data', buffer => this._dispatch(buffer)), helper.addEventListener(pipeRead, 'close', () => { + this._closed = true; helper.removeEventListeners(this._eventListeners); if (this.onclose) this.onclose.call(null); @@ -45,8 +47,10 @@ export class PipeTransport implements ConnectionTransport { } send(message: ProtocolRequest) { - this._pipeWrite!.write(JSON.stringify(message)); - this._pipeWrite!.write('\0'); + if (this._closed) + throw new Error('Pipe has been closed'); + this._pipeWrite.write(JSON.stringify(message)); + this._pipeWrite.write('\0'); } close() { diff --git a/src/server/webkit.ts b/src/server/webkit.ts index 5660d3277a..2d4fe68cf3 100644 --- a/src/server/webkit.ts +++ b/src/server/webkit.ts @@ -42,11 +42,17 @@ export class WebKit extends AbstractBrowserType { async launch(options: LaunchOptions = {}): Promise { assert(!(options as any).userDataDir, 'userDataDir option is not supported in `browserType.launch`. Use `browserType.launchPersistentContext` instead'); + const { timeout = 30000 } = options; + const deadline = TimeoutSettings.computeDeadline(timeout); const { browserServer, transport, downloadsPath, logger } = await this._launchServer(options, 'local'); - const browser = await WKBrowser.connect(transport!, logger, options.slowMo, false); - browser._ownedServer = browserServer; - browser._downloadsPath = downloadsPath; - return browser; + return await browserServer._initializeOrClose(deadline, async () => { + if ((options as any).__testHookBeforeCreateBrowser) + await (options as any).__testHookBeforeCreateBrowser(); + const browser = await WKBrowser.connect(transport!, logger, options.slowMo, false); + browser._ownedServer = browserServer; + browser._downloadsPath = downloadsPath; + return browser; + }); } async launchServer(options: LaunchServerOptions = {}): Promise { @@ -60,12 +66,16 @@ export class WebKit extends AbstractBrowserType { } = options; const deadline = TimeoutSettings.computeDeadline(timeout); const { transport, browserServer, logger } = await this._launchServer(options, 'persistent', userDataDir); - const browser = await WKBrowser.connect(transport!, logger, slowMo, true); - browser._ownedServer = browserServer; - const context = browser._defaultContext!; - if (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs)) - await helper.waitWithTimeout(context._loadDefaultContext(), 'first page', helper.timeUntilDeadline(deadline)); - return context; + return await browserServer._initializeOrClose(deadline, async () => { + if ((options as any).__testHookBeforeCreateBrowser) + await (options as any).__testHookBeforeCreateBrowser(); + const browser = await WKBrowser.connect(transport!, logger, slowMo, true); + browser._ownedServer = browserServer; + const context = browser._defaultContext!; + if (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs)) + await context._loadDefaultContext(); + return context; + }); } private async _launchServer(options: LaunchServerOptions, launchType: LaunchType, userDataDir?: string): Promise<{ browserServer: BrowserServer, transport?: ConnectionTransport, downloadsPath: string, logger: InnerLogger }> { @@ -123,7 +133,8 @@ export class WebKit extends AbstractBrowserType { }, }); - // For local launch scenario close will terminate the browser process. + // Note: it is important to define these variables before launchProcess, so that we don't get + // "Cannot access 'browserServer' before initialization" if something went wrong. let transport: ConnectionTransport | undefined = undefined; let browserServer: BrowserServer | undefined = undefined; const stdio = launchedProcess.stdio as unknown as [NodeJS.ReadableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.WritableStream, NodeJS.ReadableStream]; diff --git a/test/launcher.spec.js b/test/launcher.spec.js index db44884b31..98e07f938c 100644 --- a/test/launcher.spec.js +++ b/test/launcher.spec.js @@ -51,6 +51,17 @@ describe('Playwright', function() { await browserType.launch(options).catch(e => waitError = e); expect(waitError.message).toContain('Failed to launch'); }); + it('should handle timeout', async({browserType, defaultBrowserOptions}) => { + const options = { ...defaultBrowserOptions, timeout: 1000, __testHookBeforeCreateBrowser: () => new Promise(f => setTimeout(f, 2000)) }; + const error = await browserType.launch(options).catch(e => e); + expect(error.message).toBe('Waiting for the browser to launch failed: timeout exceeded. Re-run with the DEBUG=pw:browser* env variable to see the debug log.'); + }); + it('should handle exception', async({browserType, defaultBrowserOptions}) => { + const e = new Error('Dummy'); + const options = { ...defaultBrowserOptions, __testHookBeforeCreateBrowser: () => { throw e; } }; + const error = await browserType.launch(options).catch(e => e); + expect(error).toBe(e); + }); }); describe('browserType.launchPersistentContext', function() { @@ -87,6 +98,21 @@ describe('Playwright', function() { await browserContext.close(); await removeUserDataDir(userDataDir); }); + it('should handle timeout', async({browserType, defaultBrowserOptions}) => { + const userDataDir = await makeUserDataDir(); + const options = { ...defaultBrowserOptions, timeout: 1000, __testHookBeforeCreateBrowser: () => new Promise(f => setTimeout(f, 2000)) }; + const error = await browserType.launchPersistentContext(userDataDir, options).catch(e => e); + expect(error.message).toBe('Waiting for the browser to launch failed: timeout exceeded. Re-run with the DEBUG=pw:browser* env variable to see the debug log.'); + await removeUserDataDir(userDataDir); + }); + it('should handle exception', async({browserType, defaultBrowserOptions}) => { + const userDataDir = await makeUserDataDir(); + const e = new Error('Dummy'); + const options = { ...defaultBrowserOptions, __testHookBeforeCreateBrowser: () => { throw e; } }; + const error = await browserType.launchPersistentContext(userDataDir, options).catch(e => e); + expect(error).toBe(e); + await removeUserDataDir(userDataDir); + }); }); describe('browserType.launchServer', function() {