chore: Optimize process events handling (#415)

Author: mykola-mokhnach

.github/workflows/unit-test.yml

@@ -18,13 +18,16 @@ jobs:
     strategy:
       matrix:
         node-version: ${{ fromJSON(needs.prepare_matrix.outputs.versions) }}
+      # TODO: Remove after node 22.5.0 is fixed
+      fail-fast: false
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v3
     - name: Use Node.js
       uses: actions/setup-node@v3
       with:
         node-version: ${{ matrix.node-version }}
+        check-latest: true
     - run: npm install
       name: Install dev dependencies
     - run: npm run lint

lib/chromedriver.js

@@ -513,69 +513,71 @@ export class Chromedriver extends events.EventEmitter {
 
   /**
    * Sync the WebDriver protocol if current on going protocol is W3C or MJSONWP.
-   * Does nothing if this.driverVersion was null.
+   * Does nothing if this.driverVersion is null.
+   *
+   * @returns {typeof PROTOCOLS[keyof typeof PROTOCOLS]}
    */
   syncProtocol() {
-    if (this.driverVersion === null) {
+    if (!this.driverVersion) {
       // Keep the default protocol if the driverVersion was unsure.
-      return;
+      return this.desiredProtocol;
     }
 
+    this.desiredProtocol = PROTOCOLS.MJSONWP;
     const coercedVersion = semver.coerce(this.driverVersion);
     if (!coercedVersion || coercedVersion.major < MIN_CD_VERSION_WITH_W3C_SUPPORT) {
-      this.log.debug(
-        `The WebDriver v. ${this.driverVersion} does not fully support ${PROTOCOLS.W3C} protocol. ` +
+      this.log.info(
+        `The ChromeDriver v. ${this.driverVersion} does not fully support ${PROTOCOLS.W3C} protocol. ` +
           `Defaulting to ${PROTOCOLS.MJSONWP}`,
       );
-      return;
+      return this.desiredProtocol;
     }
-
     // Check only chromeOptions for now.
     const chromeOptions = getCapValue(this.capabilities, 'chromeOptions', {});
     if (chromeOptions.w3c === false) {
       this.log.info(
-        `The WebDriver v. ${this.driverVersion} supports ${PROTOCOLS.W3C} protocol, ` +
+        `The ChromeDriver v. ${this.driverVersion} supports ${PROTOCOLS.W3C} protocol, ` +
           `but ${PROTOCOLS.MJSONWP} one has been explicitly requested`,
       );
-      return;
+      return this.desiredProtocol;
     }
+
     this.desiredProtocol = PROTOCOLS.W3C;
-    // given caps might not be properly prefixed
-    // so we try to fix them in order to properly init
-    // the new W3C session
-    this.capabilities = toW3cCapNames(this.capabilities);
+    this.log.info(`Set ChromeDriver communication protocol to ${PROTOCOLS.W3C}`);
+    return this.desiredProtocol;
   }
 
   /**
    * Sync the protocol by reading the given output
    *
-   * @param {string} out The output of WebDriver process start
+   * @param {string} line The output of ChromeDriver process
+   * @returns {typeof PROTOCOLS[keyof typeof PROTOCOLS] | null}
    */
-  detectWebDriverProtocol(out) {
+  detectWebDriverProtocol(line) {
     if (this.driverVersion) {
-      // Nothing is done if the protocol was already detected
-      return;
+      return this.syncProtocol();
     }
 
     // also print chromedriver version to logs
     // will output something like
     //  Starting ChromeDriver 2.33.506106 (8a06c39c4582fbfbab6966dbb1c38a9173bfb1a2) on port 9515
     // Or MSEdge:
     //  Starting Microsoft Edge WebDriver 111.0.1661.41 (57be51b50d1be232a9e8186a10017d9e06b1fd16) on port 9515
-    const match = WEBDRIVER_VERSION_PATTERN.exec(out);
+    const match = WEBDRIVER_VERSION_PATTERN.exec(line);
     if (match && match.length === 3) {
       this.log.debug(`${match[1]} version: '${match[2]}'`);
       this.driverVersion = match[2];
       try {
-        this.syncProtocol();
+        return this.syncProtocol();
       } catch (e) {
         this.driverVersion = null;
-        this.log.error(`Failed to sync the protocol as '${e}'. Stopping the driver process.`);
+        this.log.error(`Stopping the chromedriver process. Cannot determinate the protocol: ${e}`);
         this.stop();
       }
       // Does not print else condition log since the log could be
       // very noisy when this.verbose option is true.
     }
+    return null;
   }
 
   /**
@@ -615,7 +617,9 @@ export class Chromedriver extends events.EventEmitter {
     const startDetector = /** @param {string} stdout */ (stdout) => stdout.startsWith('Starting ');
 
     let processIsAlive = false;
+    /** @type {string|undefined} */
     let webviewVersion;
+    let didDetectProtocol = false;
     try {
       const chromedriverPath = await this.initChromedriverPath();
       await this.killAll();
@@ -625,40 +629,45 @@ export class Chromedriver extends events.EventEmitter {
       processIsAlive = true;
 
       // handle log output
-      this.proc.on('output', (stdout, stderr) => {
-        // if the cd output is not printed, find the chrome version and print
-        // will get a response like
-        //   DevTools response: {
-        //      "Android-Package": "io.appium.sampleapp",
-        //      "Browser": "Chrome/55.0.2883.91",
-        //      "Protocol-Version": "1.2",
-        //      "User-Agent": "...",
-        //      "WebKit-Version": "537.36"
-        //   }
-        const out = stdout + stderr;
-        let match = /"Browser": "(.*)"/.exec(out);
-        if (match) {
-          webviewVersion = match[1];
-          this.log.debug(`Webview version: '${webviewVersion}'`);
-        }
-
-        this.detectWebDriverProtocol(out);
+      for (const streamName of ['stderr', 'stdout']) {
+        this.proc.on(`line-${streamName}`, (line) => {
+          // if the cd output is not printed, find the chrome version and print
+          // will get a response like
+          //   DevTools response: {
+          //      "Android-Package": "io.appium.sampleapp",
+          //      "Browser": "Chrome/55.0.2883.91",
+          //      "Protocol-Version": "1.2",
+          //      "User-Agent": "...",
+          //      "WebKit-Version": "537.36"
+          //   }
+          if (!webviewVersion) {
+            const match = /"Browser": "([^"]+)"/.exec(line);
+            if (match) {
+              webviewVersion = match[1];
+              this.log.debug(`Webview version: '${webviewVersion}'`);
+            }
+          }
 
-        // give the output if it is requested
-        if (this.verbose) {
-          for (let line of (stdout || '').trim().split('\n')) {
-            if (!line.trim().length) continue; // eslint-disable-line curly
-            this.log.debug(`[STDOUT] ${line}`);
+          if (!didDetectProtocol) {
+            const proto = this.detectWebDriverProtocol(line);
+            if (proto === PROTOCOLS.W3C) {
+              // given caps might not be properly prefixed
+              // so we try to fix them in order to properly init
+              // the new W3C session
+              this.capabilities = toW3cCapNames(this.capabilities);
+            }
+            didDetectProtocol = true;
           }
-          for (let line of (stderr || '').trim().split('\n')) {
-            if (!line.trim().length) continue; // eslint-disable-line curly
-            this.log.error(`[STDERR] ${line}`);
+
+          if (this.verbose) {
+            // give the output if it is requested
+            this.log.debug(`[${streamName.toUpperCase()}] ${line}`);
           }
-        }
-      });
+        });
+      }
 
       // handle out-of-bound exit by simply emitting a stopped state
-      this.proc.on('exit', (code, signal) => {
+      this.proc.once('exit', (code, signal) => {
         this.driverVersion = null;
         processIsAlive = false;
         if (
@@ -670,6 +679,8 @@ export class Chromedriver extends events.EventEmitter {
           this.log.error(msg);
           this.changeState(Chromedriver.STATE_STOPPED);
         }
+        this.proc?.removeAllListeners();
+        this.proc = null;
       });
       this.log.info(`Spawning chromedriver with: ${this.chromedriver} ${args.join(' ')}`);
       // start subproc and wait for startDetector
@@ -685,6 +696,8 @@ export class Chromedriver extends events.EventEmitter {
       if (processIsAlive) {
         await this.proc?.stop();
       }
+      this.proc?.removeAllListeners();
+      this.proc = null;
 
       let message = '';
       // often the user's Chrome version is not supported by the version of Chromedriver
@@ -776,7 +789,11 @@ export class Chromedriver extends events.EventEmitter {
       }
     };
     await runSafeStep(() => this.jwproxy.command('', 'DELETE'));
-    await runSafeStep(() => this.proc?.stop('SIGTERM', 20000));
+    await runSafeStep(() => {
+      this.proc?.stop('SIGTERM', 20000);
+      this.proc?.removeAllListeners();
+      this.proc = null;
+    });
     this.log.prefix = generateLogPrefix(this);
     if (emitStates) {
       this.changeState(Chromedriver.STATE_STOPPED);

package.json

@@ -54,7 +54,7 @@
     "lodash": "^4.17.4",
     "semver": "^7.0.0",
     "source-map-support": "^0.x",
-    "teen_process": "^2.0.0",
+    "teen_process": "^2.2.0",
     "xpath": "^0.x"
   },
   "scripts": {