chore: migrate waitForEvent to Progress (#2483)

Drive-by: remove/simplify some helper code.
microsoft · Jun 5, 2020 · 3000997 · 3000997
1 parent fb058ff
commit 3000997
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 102 deletions.
diff --git 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
@@ -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<Error>(() => 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<any> {
+    const options = typeof optionsOrPredicate === 'function' ? { predicate: optionsOrPredicate } : optionsOrPredicate;
+    const { predicate = () => true } = options;
+
+    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));
 
-  async waitForEvent(event: string, optionsOrPredicate: Function|{ predicate?: Function, timeout?: number } = {}): Promise<any> {
-    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 result = await promise;
+      helper.removeEventListeners(listeners);
+      return result;
+    });
   }
 }
diff --git 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<Error>): Promise<any> {
-    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<T>(promise: Promise<T>, taskName: string, deadline: number, debugName: string): Promise<T> {
-    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<T>((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<T extends types.TimeoutOptions>(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
@@ -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<network.Request> {
-    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<network.Response> {
-    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<network.Response | null> {

diff --git a/src/server/browserServer.ts b/src/server/browserServer.ts
@@ -83,10 +83,16 @@ export class BrowserServer extends EventEmitter {
   }
 
   async _closeOrKill(deadline: number): Promise<void> {
+    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
@@ -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;
   }
 }