From 300099734c051357f50fbfbc7ca003028856f2ef Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 5 Jun 2020 14:14:19 -0700 Subject: [PATCH] chore: migrate waitForEvent to Progress (#2483) Drive-by: remove/simplify some helper code. --- src/browserContext.ts | 8 +++- src/extendedEventEmitter.ts | 44 +++++++++++++++------ src/helper.ts | 76 ------------------------------------- src/page.ts | 20 ++++++---- src/server/browserServer.ts | 10 ++++- src/server/electron.ts | 8 +++- 6 files changed, 64 insertions(+), 102 deletions(-) diff --git a/src/browserContext.ts b/src/browserContext.ts index 9fb004ea01..3d35b3e139 100644 --- a/src/browserContext.ts +++ b/src/browserContext.ts @@ -104,8 +104,12 @@ export abstract class BrowserContextBase extends ExtendedEventEmitter implements return event === Events.BrowserContext.Close ? super._abortPromiseForEvent(event) : this._closePromise; } - protected _computeDeadline(options?: types.TimeoutOptions): number { - return this._timeoutSettings.computeDeadline(options); + protected _getLogger(): InnerLogger { + return this._logger; + } + + protected _getTimeoutSettings(): TimeoutSettings { + return this._timeoutSettings; } _browserClosed() { diff --git a/src/extendedEventEmitter.ts b/src/extendedEventEmitter.ts index db4addd849..7a76f4ed10 100644 --- a/src/extendedEventEmitter.ts +++ b/src/extendedEventEmitter.ts @@ -15,23 +15,43 @@ */ import { EventEmitter } from 'events'; -import { helper } from './helper'; -import { TimeoutOptions } from './types'; +import { helper, RegisteredListener } from './helper'; +import { ProgressController } from './progress'; +import { InnerLogger } from './logger'; +import { TimeoutSettings } from './timeoutSettings'; -export class ExtendedEventEmitter extends EventEmitter { +export abstract class ExtendedEventEmitter extends EventEmitter { protected _abortPromiseForEvent(event: string) { return new Promise(() => void 0); } + protected abstract _getLogger(): InnerLogger; + protected abstract _getTimeoutSettings(): TimeoutSettings; - protected _computeDeadline(options?: TimeoutOptions): number { - throw new Error('unimplemented'); - } + async waitForEvent(event: string, optionsOrPredicate: Function | { predicate?: Function, timeout?: number } = {}): Promise { + const options = typeof optionsOrPredicate === 'function' ? { predicate: optionsOrPredicate } : optionsOrPredicate; + const { predicate = () => true } = options; - async waitForEvent(event: string, optionsOrPredicate: Function|{ predicate?: Function, timeout?: number } = {}): Promise { - const deadline = this._computeDeadline(typeof optionsOrPredicate === 'function' ? undefined : optionsOrPredicate); - const { - predicate = () => true, - } = typeof optionsOrPredicate === 'function' ? {predicate: optionsOrPredicate} : optionsOrPredicate; - return helper.waitForEvent(this, event, predicate, deadline, this._abortPromiseForEvent(event)); + const progressController = new ProgressController(options, this._getLogger(), this._getTimeoutSettings()); + this._abortPromiseForEvent(event).then(error => progressController.abort(error)); + + return progressController.run(async progress => { + const listeners: RegisteredListener[] = []; + const promise = new Promise((resolve, reject) => { + listeners.push(helper.addEventListener(this, event, eventArg => { + try { + if (!predicate(eventArg)) + return; + resolve(eventArg); + } catch (e) { + reject(e); + } + })); + }); + progress.cleanupWhenAborted(() => helper.removeEventListeners(listeners)); + + const result = await promise; + helper.removeEventListeners(listeners); + return result; + }); } } diff --git a/src/helper.ts b/src/helper.ts index 3c533117ae..a96e6bd52c 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -19,9 +19,7 @@ import * as crypto from 'crypto'; import { EventEmitter } from 'events'; import * as fs from 'fs'; import * as util from 'util'; -import { TimeoutError } from './errors'; import * as types from './types'; -import { ChildProcess, execSync } from 'child_process'; export type RegisteredListener = { emitter: EventEmitter; @@ -124,59 +122,6 @@ class Helper { return typeof obj === 'boolean' || obj instanceof Boolean; } - static async waitForEvent( - emitter: EventEmitter, - eventName: (string | symbol), - predicate: Function, - deadline: number, - abortPromise: Promise): Promise { - let resolveCallback: (event: any) => void = () => {}; - let rejectCallback: (error: any) => void = () => {}; - const promise = new Promise((resolve, reject) => { - resolveCallback = resolve; - rejectCallback = reject; - }); - - // Add listener. - const listener = Helper.addEventListener(emitter, eventName, event => { - try { - if (!predicate(event)) - return; - resolveCallback(event); - } catch (e) { - rejectCallback(e); - } - }); - - // Reject upon timeout. - const eventTimeout = setTimeout(() => { - rejectCallback(new TimeoutError(`Timeout exceeded while waiting for ${String(eventName)}`)); - }, helper.timeUntilDeadline(deadline)); - - // Reject upon abort. - abortPromise.then(rejectCallback); - - try { - return await promise; - } finally { - Helper.removeEventListeners([listener]); - clearTimeout(eventTimeout); - } - } - - 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=${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 { - return await Promise.race([promise, timeoutPromise]); - } finally { - if (timeoutTimer) - clearTimeout(timeoutTimer); - } - } - static globToRegex(glob: string): RegExp { const tokens = ['^']; let inGroup; @@ -321,31 +266,10 @@ class Helper { return seconds * 1000 + (nanoseconds / 1000000 | 0); } - static isPastDeadline(deadline: number) { - return deadline !== Number.MAX_SAFE_INTEGER && this.monotonicTime() >= deadline; - } - static timeUntilDeadline(deadline: number): number { return Math.min(deadline - this.monotonicTime(), 2147483647); // 2^31-1 safe setTimeout in Node. } - static optionsWithUpdatedTimeout(options: T | undefined, deadline: number): T { - return { ...(options || {}) as T, timeout: this.timeUntilDeadline(deadline) }; - } - - static killProcess(proc: ChildProcess) { - if (proc.pid && !proc.killed) { - try { - if (process.platform === 'win32') - execSync(`taskkill /pid ${proc.pid} /T /F`); - else - process.kill(-proc.pid, 'SIGKILL'); - } catch (e) { - // the process might have already stopped - } - } - } - static getViewportSizeFromWindowFeatures(features: string[]): types.Size | null { const widthString = features.find(f => f.startsWith('width=')); const heightString = features.find(f => f.startsWith('height=')); diff --git a/src/page.ts b/src/page.ts index 945bca1152..59488fcaba 100644 --- a/src/page.ts +++ b/src/page.ts @@ -140,8 +140,12 @@ export class Page extends ExtendedEventEmitter implements InnerLogger { return this._disconnectedPromise; } - protected _computeDeadline(options?: types.TimeoutOptions): number { - return this._timeoutSettings.computeDeadline(options); + protected _getLogger(): InnerLogger { + return this; + } + + protected _getTimeoutSettings(): TimeoutSettings { + return this._timeoutSettings; } _didClose() { @@ -314,21 +318,21 @@ export class Page extends ExtendedEventEmitter implements InnerLogger { } async waitForRequest(urlOrPredicate: string | RegExp | ((r: network.Request) => boolean), options: types.TimeoutOptions = {}): Promise { - const deadline = this._timeoutSettings.computeDeadline(options); - return helper.waitForEvent(this, Events.Page.Request, (request: network.Request) => { + const predicate = (request: network.Request) => { if (helper.isString(urlOrPredicate) || helper.isRegExp(urlOrPredicate)) return helper.urlMatches(request.url(), urlOrPredicate); return urlOrPredicate(request); - }, deadline, this._disconnectedPromise); + }; + return this.waitForEvent(Events.Page.Request, { predicate, timeout: options.timeout }); } async waitForResponse(urlOrPredicate: string | RegExp | ((r: network.Response) => boolean), options: types.TimeoutOptions = {}): Promise { - const deadline = this._timeoutSettings.computeDeadline(options); - return helper.waitForEvent(this, Events.Page.Response, (response: network.Response) => { + const predicate = (response: network.Response) => { if (helper.isString(urlOrPredicate) || helper.isRegExp(urlOrPredicate)) return helper.urlMatches(response.url(), urlOrPredicate); return urlOrPredicate(response); - }, deadline, this._disconnectedPromise); + }; + return this.waitForEvent(Events.Page.Response, { predicate, timeout: options.timeout }); } async goBack(options?: types.NavigateOptions): Promise { diff --git a/src/server/browserServer.ts b/src/server/browserServer.ts index 1b0f36ba0b..125a822212 100644 --- a/src/server/browserServer.ts +++ b/src/server/browserServer.ts @@ -83,10 +83,16 @@ export class BrowserServer extends EventEmitter { } async _closeOrKill(deadline: number): Promise { + let timer: NodeJS.Timer; try { - await helper.waitWithDeadline(this.close(), '', deadline, ''); // The error message is ignored. + await Promise.race([ + this.close(), + new Promise((resolve, reject) => timer = setTimeout(reject, helper.timeUntilDeadline(deadline))), + ]); } catch (ignored) { - await this.kill(); // Make sure to await actual process exit. + await this.kill().catch(ignored => {}); // Make sure to await actual process exit. + } finally { + clearTimeout(timer!); } } } diff --git a/src/server/electron.ts b/src/server/electron.ts index a185c61247..4c35842394 100644 --- a/src/server/electron.ts +++ b/src/server/electron.ts @@ -153,8 +153,12 @@ export class ElectronApplication extends ExtendedEventEmitter { return this._nodeElectronHandle!.evaluateHandle(pageFunction, arg); } - protected _computeDeadline(options?: types.TimeoutOptions): number { - return this._timeoutSettings.computeDeadline(options); + protected _getLogger(): InnerLogger { + return this._logger; + } + + protected _getTimeoutSettings(): TimeoutSettings { + return this._timeoutSettings; } }