fix(core): Fix a bug that BrowserPool hangs

packages/connection/src/index.ts

@@ -97,7 +97,9 @@ export class Connection {
     try {
       debug('disconnect server connection... (pid: "%f")', this._proc.pid);
 
-      this._proc.kill();
+      const result = this._proc.kill('SIGKILL');
+
+      debug('kill=%s', result);
     } catch (e) {
       debug(e);
     }

packages/core/src/__tests__/browser-pool.test.ts

@@ -17,9 +17,12 @@ describe('BrowserPool', () => {
         const closer = {
           close: async () => {},
         } as any;
-        this._status = 'idle';
+
         this._browser = closer;
         this._page = closer;
+
+        this.close = jest.fn().mockReturnValue(Promise.resolve());
+
         return this;
       }
     }
@@ -60,7 +63,7 @@ describe('BrowserPool', () => {
 
     await pool.bootstrap(4);
 
-    expect((pool as any)._browsers.length).toBe(4);
+    expect((pool as any)._available.size).toBe(4);
   });
 
   /**
@@ -76,8 +79,8 @@ describe('BrowserPool', () => {
 
     await pool.bootstrap(2);
 
-    const mock = jest.fn().mockResolvedValue('success');
-    expect(await pool.execute(mock)).toBe('success');
+    const mock = jest.fn().mockReturnValue(Promise.resolve());
+    await pool.execute(mock);
     expect(mock).toBeCalled();
   });
 
@@ -90,7 +93,9 @@ describe('BrowserPool', () => {
     await pool.bootstrap(2);
     await pool.terminate();
 
-    expect((pool as any)._browsers[0].status()).toBe('closed');
-    expect((pool as any)._browsers[1].status()).toBe('closed');
+    const browsers = [...(pool as any)._available];
+
+    expect(browsers[0].close).toBeCalled();
+    expect(browsers[1].close).toBeCalled();
   });
 });

packages/core/src/__tests__/browser.test.ts

@@ -23,20 +23,12 @@ describe('Browser', () => {
 
   test('launch to close', async () => {
     const browser = new Browser(10);
-    expect(browser.status()).toBe('closed');
 
     const res = await browser.launch(launchOptions);
-    expect(browser.status()).toBe('idle');
-    expect(res).toBe(browser);
-    expect((browser.page() as any).constructor.name).toBe('Page');
-
-    browser.lock();
-    expect(browser.status()).toBe('active');
 
-    browser.release();
-    expect(browser.status()).toBe('idle');
+    expect(res).toBe(browser);
+    expect(((await browser.page()) as any).constructor.name).toBe('Page');
 
     await browser.close();
-    expect(browser.status()).toBe('closed');
   });
 });

packages/core/src/acot.ts

@@ -49,8 +49,8 @@ export class Acot implements Core {
       origin: config.origin ?? '',
       parallel: Math.max(config.parallel ?? 1, 1),
       plugins: config.plugins ?? [],
-      browserTimeout: 1000 * 30,
-      readyTimeout: 1000 * 30,
+      browserTimeout: config.browserTimeout ?? 1000 * 30,
+      readyTimeout: config.readyTimeout ?? 1000 * 30,
     };
 
     this._store = new RuleStore();

packages/core/src/browser-pool.ts

@@ -1,14 +1,31 @@
 import type { LaunchOptions } from 'puppeteer-core';
 import { Browser } from './browser';
 
+const WORK_INTERVAL = 30;
+
+export type BrowserTask<T extends any[] = any[]> = (
+  browser: Browser,
+  ...args: T
+) => Promise<void>;
+
+export type BrowserJob = {
+  fn: BrowserTask;
+  args: any[];
+  resolve: (value?: any) => void;
+  reject: (reason?: any) => void;
+};
+
 export type BrowserPoolConfig = {
   launchOptions: LaunchOptions;
   timeout: number;
 };
 
 export class BrowserPool {
   private _config: BrowserPoolConfig;
-  private _browsers: Browser[] = [];
+  private _queue: BrowserJob[] = [];
+  private _available: Set<Browser> = new Set();
+  private _busy: Set<Browser> = new Set();
+  private _interval: NodeJS.Timeout | null = null;
 
   public constructor(config: BrowserPoolConfig) {
     this._config = {
@@ -33,57 +50,56 @@ export class BrowserPool {
   public async bootstrap(parallel: number): Promise<void> {
     const { launchOptions } = this._config;
 
-    this._browsers = await Promise.all(
-      Array.from(Array(parallel || 1)).map((_, i) =>
-        new Browser(i).launch(launchOptions),
-      ),
+    await Promise.all(
+      Array.from(Array(parallel || 1)).map(async (_, i) => {
+        this._available.add(await new Browser(i).launch(launchOptions));
+      }),
     );
+
+    this._interval = setInterval(() => this._work(), WORK_INTERVAL);
   }
 
   public async terminate(): Promise<void> {
-    await Promise.all(this._browsers.map((browser) => browser.close()));
+    if (this._interval != null) {
+      clearInterval(this._interval);
+    }
+
+    await Promise.all(
+      [...this._available, ...this._busy].map((browser) => browser.close()),
+    );
   }
 
-  public async execute<T = void, A extends any[] = any[]>(
-    callback: (browser: Browser, ...args: A) => Promise<T>,
-    ...args: A
+  public execute<T extends any[] = any[]>(
+    fn: BrowserTask<T>,
+    ...args: T
   ): Promise<T> {
-    const browser = await this._waitForIdleBrowser();
-    let result: T;
-
-    try {
-      browser.lock();
-      result = await callback(browser, ...args);
-    } finally {
-      browser.release();
-    }
-
-    return result;
+    return new Promise((resolve, reject) => {
+      this._queue.push({ fn: fn as BrowserTask, args, resolve, reject });
+    });
   }
 
-  private _waitForIdleBrowser(): Promise<Browser> {
-    const { timeout } = this._config;
-    const start = Date.now();
+  private async _work(): Promise<void> {
+    if (this._available.size === 0 || this._queue.length === 0) {
+      return;
+    }
 
-    return new Promise((resolve, reject) => {
-      const callback = () => {
-        const diff = Date.now() - start;
-        if (diff >= timeout) {
-          return reject(new Error(`Browser idle timed out. (${timeout} ms)`));
-        }
+    const job = this._queue.shift()!;
+    const browser = [...this._available.values()].shift()!;
 
-        const idle = this._browsers.find(
-          (browser) => browser.status() === 'idle',
-        );
+    this._busy.add(browser);
 
-        if (idle) {
-          return resolve(idle);
-        }
+    try {
+      await job.fn(browser, ...job.args);
+      job.resolve();
+    } catch (e) {
+      job.reject(e);
+    }
 
-        setImmediate(callback);
-      };
+    this._busy.delete(browser);
+    this._available.add(browser);
 
-      setImmediate(callback);
-    });
+    if (this._queue.length > 0) {
+      this._work();
+    }
   }
 }

packages/core/src/browser.ts

@@ -6,72 +6,59 @@ import type {
 import { launch } from 'puppeteer-core';
 const debug = require('debug')('acot:core');
 
-export type BrowserStatus = 'idle' | 'active' | 'closed';
-
 export class Browser {
   protected _id: number;
-  protected _browser!: PuppeteerBrowser;
-  protected _page!: Page;
-  protected _status: BrowserStatus = 'closed';
+  protected _browser: PuppeteerBrowser | null;
+  protected _page: Page | null;
 
   public constructor(id: number) {
     this._id = id;
+    this._browser = null;
+    this._page = null;
   }
 
   public id(): number {
     return this._id;
   }
 
-  public status(): BrowserStatus {
-    return this._status;
-  }
-
-  public lock(): void {
-    this._status = 'active';
-  }
-
-  public release(): void {
-    this._status = 'idle';
-  }
-
-  public page(): Page {
-    return this._page;
-  }
+  public async page(): Promise<Page> {
+    if (this._browser == null) {
+      throw new Error(
+        'You will need to launch the Browser in order to create the page.',
+      );
+    }
 
-  public async launch(launchOptions: LaunchOptions): Promise<Browser> {
-    this._browser = await launch(launchOptions);
     this._page = await this._browser.newPage();
 
     this._page.on('console', (message) => {
       const args = message.args();
       for (const msg of args) {
-        // logger.log('console: %s', msg);
         debug('browser.console: %O', msg);
       }
     });
 
-    this._status = 'idle';
+    return this._page;
+  }
+
+  public async launch(launchOptions: LaunchOptions): Promise<Browser> {
+    this._browser = await launch(launchOptions);
 
     return this;
   }
 
   public async close(): Promise<Browser> {
     try {
-      await this._page.close();
+      await this._page?.close();
     } catch (e) {
-      // logger.log('Closing the page failed', e);
       debug(e);
     }
 
     try {
-      await this._browser.close();
+      await this._browser?.close();
     } catch (e) {
-      // logger.log('Closing the browser failed', e);
       debug(e);
     }
 
-    this._status = 'closed';
-
     return this;
   }
 }

packages/core/src/tester.ts

@@ -157,7 +157,7 @@ export class Tester {
     browser: Browser,
     [id, rule, options]: TesterRuleGroup,
   ): Promise<void> {
-    const page = browser.page();
+    const page = await browser.page();
 
     await Promise.all([
       this._config.headers && page.setExtraHTTPHeaders(this._config.headers),

packages/runner/src/base.ts

@@ -50,10 +50,22 @@ export class BaseRunner implements Runner {
     await this._setup();
     await this._connect();
     await this._collect();
-    const summary = await this._audit();
-    await this._cleanup();
 
-    return summary;
+    let summary: Summary;
+    let error: any | null = null;
+    try {
+      summary = await this._audit();
+    } catch (e) {
+      error = e;
+    } finally {
+      await this._cleanup();
+    }
+
+    if (error != null) {
+      throw error;
+    }
+
+    return summary!;
   }
 
   private async _setup(): Promise<void> {