From 0fd5078b2b94eb2d7e51e18181d8d7b95f956d72 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Tue, 31 Aug 2021 15:56:55 -0700 Subject: [PATCH] chore: simplify client-side browserType.connect routine (#8596) --- src/client/browserType.ts | 83 +++++++++++------------- src/dispatchers/browserTypeDispatcher.ts | 17 +++-- src/dispatchers/jsonPipeDispatcher.ts | 9 ++- src/protocol/channels.ts | 3 - src/protocol/protocol.yml | 2 - tests/browsertype-connect.spec.ts | 12 +++- tests/browsertype-launch-server.spec.ts | 2 +- 7 files changed, 65 insertions(+), 63 deletions(-) diff --git a/src/client/browserType.ts b/src/client/browserType.ts index 80710db6b2..084a831114 100644 --- a/src/client/browserType.ts +++ b/src/client/browserType.ts @@ -23,10 +23,10 @@ import { Connection } from './connection'; import { Events } from './events'; import { ChildProcess } from 'child_process'; import { envObjectToArray } from './clientHelper'; -import { assert, headersObjectToArray, getUserAgent } from '../utils/utils'; +import { assert, headersObjectToArray, getUserAgent, monotonicTime } from '../utils/utils'; import * as api from '../../types/types'; import { kBrowserClosedError } from '../utils/errors'; -import { ManualPromise } from '../utils/async'; +import { raceAgainstDeadline } from '../utils/async'; export interface BrowserServerLauncher { launchServer(options?: LaunchServerOptions): Promise; @@ -128,12 +128,25 @@ export class BrowserType extends ChannelOwner = {}): Promise { const logger = params.logger; return await this._wrapApiCall(async (channel: channels.BrowserTypeChannel) => { - const timeoutPromise = new ManualPromise(); - const timer = params.timeout ? setTimeout(() => timeoutPromise.reject(new Error(`Timeout ${params.timeout}ms exceeded.`)), params.timeout) : undefined; - + const deadline = params.timeout ? monotonicTime() + params.timeout : 0; + let browser: Browser; const { pipe } = await channel.connect({ wsEndpoint, headers: params.headers, timeout: params.timeout }); - const connection = new Connection(() => pipe.close().catch(() => {})); - connection.onmessage = message => pipe.send({ message }).catch(() => { }); + const closePipe = () => pipe.close().catch(() => {}); + const connection = new Connection(closePipe); + + const onPipeClosed = () => { + // Emulate all pages, contexts and the browser closing upon disconnect. + for (const context of browser?.contexts() || []) { + for (const page of context.pages()) + page._onClose(); + context._onClose(); + } + browser?._didClose(); + connection.didDisconnect(kBrowserClosedError); + }; + pipe.on('closed', onPipeClosed); + connection.onmessage = message => pipe.send({ message }).catch(onPipeClosed); + pipe.on('message', ({ message }) => { try { if (!connection!.isDisconnected()) @@ -141,64 +154,42 @@ export class BrowserType extends ChannelOwner {}); + closePipe(); } }); - const successPromise = new Promise(async (fulfill, reject) => { - if ((params as any).__testHookBeforeCreateBrowser) { - try { + const createBrowserPromise = new Promise(async (fulfill, reject) => { + try { + // For tests. + if ((params as any).__testHookBeforeCreateBrowser) await (params as any).__testHookBeforeCreateBrowser(); - } catch (e) { - reject(e); - } - } - const prematureCloseListener = (params: { error?: channels.SerializedError }) => { - reject(new Error(`WebSocket server disconnected ${params.error!.error?.message}`)); - }; - pipe.on('closed', prematureCloseListener); - pipe.on('opened', async () => { const playwright = await connection!.initializePlaywright(); - if (!playwright._initializer.preLaunchedBrowser) { reject(new Error('Malformed endpoint. Did you use launchServer method?')); - pipe.close().catch(() => {}); + closePipe(); return; } - - const browser = Browser.from(playwright._initializer.preLaunchedBrowser!); + browser = Browser.from(playwright._initializer.preLaunchedBrowser!); browser._logger = logger; browser._remoteType = 'owns-connection'; browser._setBrowserType((playwright as any)[browser._name]); - const closeListener = (param: { error?: Error }) => { - // Emulate all pages, contexts and the browser closing upon disconnect. - for (const context of browser.contexts()) { - for (const page of context.pages()) - page._onClose(); - context._onClose(); - } - browser._didClose(); - connection.didDisconnect(kBrowserClosedError); - if (param.error) - reject(new Error(param.error + '. Most likely ws endpoint is incorrect')); - }; - pipe.off('closed', prematureCloseListener); - pipe.on('closed', closeListener); browser.on(Events.Browser.Disconnected, () => { playwright._cleanup(); - pipe.off('closed', closeListener); - pipe.close().catch(() => {}); + closePipe(); }); fulfill(browser); - }); + } catch (e) { + reject(e); + } }); - try { - return await Promise.race([successPromise, timeoutPromise]); - } finally { - if (timer) - clearTimeout(timer); + const result = await raceAgainstDeadline(createBrowserPromise, deadline); + if (result.result) { + return result.result; + } else { + closePipe(); + throw new Error(`Timeout ${params.timeout}ms exceeded`); } }, logger); } diff --git a/src/dispatchers/browserTypeDispatcher.ts b/src/dispatchers/browserTypeDispatcher.ts index b9df812a65..22c7538c61 100644 --- a/src/dispatchers/browserTypeDispatcher.ts +++ b/src/dispatchers/browserTypeDispatcher.ts @@ -23,6 +23,7 @@ import { CallMetadata } from '../server/instrumentation'; import WebSocket from 'ws'; import { JsonPipeDispatcher } from '../dispatchers/jsonPipeDispatcher'; import { getUserAgent, makeWaitForNextTask } from '../utils/utils'; +import { ManualPromise } from '../utils/async'; export class BrowserTypeDispatcher extends Dispatcher implements channels.BrowserTypeChannel { constructor(scope: DispatcherScope, browserType: BrowserType) { @@ -59,13 +60,21 @@ export class BrowserTypeDispatcher extends Dispatcher pipe.wasOpened()); + const openPromise = new ManualPromise<{ pipe: JsonPipeDispatcher }>(); + ws.on('open', () => openPromise.resolve({ pipe })); ws.on('close', () => pipe.wasClosed()); - ws.on('error', error => pipe.wasClosed(error)); + ws.on('error', error => { + if (openPromise.isDone()) { + pipe.wasClosed(error); + } else { + pipe.dispose(); + openPromise.reject(error); + } + }); pipe.on('close', () => ws.close()); pipe.on('message', message => ws.send(JSON.stringify(message))); ws.addEventListener('message', event => { @@ -77,6 +86,6 @@ export class BrowserTypeDispatcher extends Dispatcher void): this; on(event: 'message', callback: (params: JsonPipeMessageEvent) => void): this; on(event: 'closed', callback: (params: JsonPipeClosedEvent) => void): this; send(params: JsonPipeSendParams, metadata?: Metadata): Promise; close(params?: JsonPipeCloseParams, metadata?: Metadata): Promise; } -export type JsonPipeOpenedEvent = {}; export type JsonPipeMessageEvent = { message: any, }; @@ -3547,7 +3545,6 @@ export type JsonPipeCloseOptions = {}; export type JsonPipeCloseResult = void; export interface JsonPipeEvents { - 'opened': JsonPipeOpenedEvent; 'message': JsonPipeMessageEvent; 'closed': JsonPipeClosedEvent; } diff --git a/src/protocol/protocol.yml b/src/protocol/protocol.yml index ad660f135b..aa3ca05828 100644 --- a/src/protocol/protocol.yml +++ b/src/protocol/protocol.yml @@ -2852,8 +2852,6 @@ JsonPipe: events: - opened: - message: parameters: message: json diff --git a/tests/browsertype-connect.spec.ts b/tests/browsertype-connect.spec.ts index 4a0f6f8c36..d1b9f792d7 100644 --- a/tests/browsertype-connect.spec.ts +++ b/tests/browsertype-connect.spec.ts @@ -64,12 +64,20 @@ test('should be able to connect two browsers at the same time', async ({browserT await browser2.close(); }); -test('should timeout while connecting', async ({browserType, startRemoteServer, server}) => { +test('should timeout in socket while connecting', async ({browserType, startRemoteServer, server}) => { + const e = await browserType.connect({ + wsEndpoint: `ws://localhost:${server.PORT}/ws`, + timeout: 1, + }).catch(e => e); + expect(e.message).toContain('browserType.connect: Opening handshake has timed out'); +}); + +test('should timeout in connect while connecting', async ({browserType, startRemoteServer, server}) => { const e = await browserType.connect({ wsEndpoint: `ws://localhost:${server.PORT}/ws`, timeout: 100, }).catch(e => e); - expect(e.message).toContain('browserType.connect: Timeout 100ms exceeded.'); + expect(e.message).toContain('browserType.connect: Timeout 100ms exceeded'); }); test('should send extra headers with connect request', async ({browserType, startRemoteServer, server}) => { diff --git a/tests/browsertype-launch-server.spec.ts b/tests/browsertype-launch-server.spec.ts index 024852629a..cf73567ea8 100644 --- a/tests/browsertype-launch-server.spec.ts +++ b/tests/browsertype-launch-server.spec.ts @@ -57,7 +57,7 @@ it.describe('launch server', () => { const browserServer = await browserType.launchServer(browserOptions); const error = await browserType.connect({ wsEndpoint: browserServer.wsEndpoint() + '-foo' }).catch(e => e); await browserServer.close(); - expect(error.message).toContain('WebSocket server disconnected'); + expect(error.message).toContain('Unexpected server response: 400'); }); it('should fire "close" event during kill', async ({browserType, browserOptions}) => {