rename options, change defaults

nodejs · Sep 29, 2023 · 038bd4c · 038bd4c
1 parent ec1fc0a
commit 038bd4c
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 44 deletions.
diff --git a/doc/api/child_process.md b/doc/api/child_process.md
@@ -1454,12 +1454,13 @@ added: REPLACEME
 -->
 
 * `options` {Object}
-  * `shouldIgnoreErrors` {boolean} **Default:** `false`
-  * `useStdErr` {boolean} **Default:** `false`
+  * `rejectIfNonZeroExitCode` {boolean} **Default:** `false`
+  * `listenTo` {string} Can be one of `'stdout'`, or `'stderr'`.
+    **Default:** `'stdout'`
 * Returns: {readline.InterfaceConstructor}
 
-Convenience method to create a `readline` interface and stream over the stdout
-(or stderr if `useStdErr` is `true`) of the process.
+Convenience method to create a `node:readline` interface and stream over the
+output of the child process.
 
 ```mjs
 import { spawn } from 'node:child_process';

diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
@@ -4,12 +4,15 @@ const {
   ArrayIsArray,
   ArrayPrototypePush,
   ArrayPrototypeReduce,
+  ArrayPrototypeShift,
   ArrayPrototypeSlice,
   FunctionPrototype,
   FunctionPrototypeCall,
   ObjectDefineProperty,
   ObjectSetPrototypeOf,
   Promise,
+  PromiseResolve,
+  PromisePrototypeThen,
   ReflectApply,
   SafePromiseRace,
   StringPrototypeSlice,
@@ -541,33 +544,71 @@ ChildProcess.prototype.unref = function() {
 };
 
 ChildProcess.prototype.readLines = function readLines(options = undefined) {
-  const { useStdErr = false, ignoreErrors = false } = options ?? kEmptyObject;
+  const { listenTo, rejectIfNonZeroExitCode = false } = options ?? kEmptyObject;
   if (options != null) {
-    validateBoolean(useStdErr, 'options.useStdErr');
-    validateBoolean(ignoreErrors, 'options.ignoreErrors');
+    if (listenTo != null) validateOneOf(listenTo, 'options.listenTo', ['stdout', 'stderr']);
+    validateBoolean(rejectIfNonZeroExitCode, 'options.rejectIfNonZeroExitCode');
+  }
+  let input;
+  switch (listenTo) {
+    // TODO(aduh95): add case 'both'
+
+    case 'stderr':
+      input = this.stderr;
+      break;
+
+    default:
+      input = this.stdout;
   }
   const rlInterface = new Interface({
     __proto__: null,
     signal: this[kSignal],
-    input: useStdErr ? this.stderr : this.stdout,
+    input,
   });
-  if (ignoreErrors) {
-    this.on('error', () => {});
-  } else {
-    const errorPromise = new Promise((_, reject) => this.once('error', reject));
-    const exitPromise = new Promise((resolve, reject) => this.once('close', (code) => {
-      if (code === 0) resolve({ done: true, value: undefined });
-      else reject(genericNodeError('Command failed', { pid: this.pid, signal: this.signalCode, status: code }));
-    }));
-    rlInterface[SymbolAsyncIterator] = function() {
-      return { __proto__: null,
-               next: () => SafePromiseRace([
-                 errorPromise,
-                 new Promise((resolve) => this.on('line', (value) => resolve({ done: false, value }))),
-                 exitPromise]),
-               [SymbolAsyncIterator]() { return this; } };
-    };
-  }
+  const errorPromise = new Promise((_, reject) => this.once('error', reject));
+  const exitPromise = new Promise((resolve, reject) => this.once('close', rejectIfNonZeroExitCode ? (code) => {
+    if (code === 0) resolve({ done: true, value: undefined });
+    else reject(genericNodeError('Command failed', { pid: this.pid, signal: this.signalCode, status: code }));
+  } : () => resolve({ done: true, value: undefined })));
+  rlInterface[SymbolAsyncIterator] = function() {
+    let resolveNext;
+    let nextPromise = new Promise((resolve) => {
+      resolveNext = resolve;
+    });
+    const buffer = ObjectSetPrototypeOf([], null);
+    this.on('line', (value) => {
+      if (resolveNext != null) {
+        resolveNext(value);
+        resolveNext = null;
+      } else {
+        ArrayPrototypePush(buffer, value);
+      }
+    },
+    );
+    return { __proto__: null,
+             next: () => SafePromiseRace([
+               errorPromise,
+               (() => {
+                 if (nextPromise != null) {
+                   const p = nextPromise;
+                   nextPromise = buffer.length === 0 ? new Promise((resolve) => {
+                     PromisePrototypeThen(p, () => {
+                       resolveNext = resolve;
+                     });
+                   }) : null;
+                   return p;
+                 }
+                 const iteratorObject = { value: ArrayPrototypeShift(buffer), done: false };
+                 if (buffer.length === 0) {
+                   nextPromise = new Promise((resolve) => {
+                     resolveNext = resolve;
+                   });
+                 }
+                 return PromiseResolve(iteratorObject);
+               })(),
+               exitPromise]),
+             [SymbolAsyncIterator]() { return this; } };
+  };
   return rlInterface;
 };
 

diff --git a/test/parallel/test-child-process-readLines.mjs b/test/parallel/test-child-process-readLines.mjs
@@ -1,13 +1,16 @@
 import * as common from '../common/index.mjs';
 
 import assert from 'node:assert';
-import { spawn, exec, execSync } from 'node:child_process';
+import { spawn, exec } from 'node:child_process';
 
 
 {
   const lines = exec(`${process.execPath} -p 42`).readLines();
 
-  lines.on('line', common.mustCall((result) => assert.strictEqual(result, '42')));
+  lines.on('line', common.mustCall((result) => {
+    assert.strictEqual(result, '42');
+    lines.on('line', common.mustNotCall('We expect only one line event'));
+  }));
 }
 
 {
@@ -21,12 +24,12 @@ import { spawn, exec, execSync } from 'node:child_process';
 {
   const cp = spawn(process.execPath, ['-p', 42]);
 
-  [0, 1, '', 'a', 0n, 1n, Symbol(), () => {}, {}, []].forEach((useStdErr) => assert.throws(
-    () => cp.readLines({ useStdErr }),
-    { code: 'ERR_INVALID_ARG_TYPE' }));
+  [0, 1, '', 'a', 0n, 1n, Symbol(), () => {}, {}, []].forEach((listenTo) => assert.throws(
+    () => cp.readLines({ listenTo }),
+    { code: 'ERR_INVALID_ARG_VALUE' }));
 
-  [0, 1, '', 'a', 0n, 1n, Symbol(), () => {}, {}, []].forEach((ignoreErrors) => assert.throws(
-    () => cp.readLines({ ignoreErrors }),
+  [0, 1, '', 'a', 0n, 1n, Symbol(), () => {}, {}, []].forEach((rejectIfNonZeroExitCode) => assert.throws(
+    () => cp.readLines({ rejectIfNonZeroExitCode }),
     { code: 'ERR_INVALID_ARG_TYPE' }));
 }
 
@@ -38,7 +41,7 @@ await assert.rejects(async () => {
 
 await assert.rejects(async () => {
   // eslint-disable-next-line no-unused-vars
-  for await (const _ of spawn(process.execPath, { signal: AbortSignal.abort() }).readLines({ ignoreErrors: true }));
+  for await (const _ of spawn(process.execPath, { signal: AbortSignal.abort() }).readLines());
 }, { name: 'AbortError' });
 
 {
@@ -48,7 +51,7 @@ await assert.rejects(async () => {
     ['-e', 'setTimeout(()=>console.log("line 2"), 10);setImmediate(()=>console.log("line 1"));'],
     { signal: ac.signal });
   await assert.rejects(async () => {
-    for await (const line of cp.readLines({ ignoreErrors: true })) {
+    for await (const line of cp.readLines()) {
       assert.strictEqual(line, 'line 1');
       ac.abort();
     }
@@ -60,30 +63,26 @@ await assert.rejects(async () => {
   const cp = spawn(process.execPath, ['-e', 'throw null']);
   await assert.rejects(async () => {
     // eslint-disable-next-line no-unused-vars
-    for await (const _ of cp.readLines());
+    for await (const _ of cp.readLines({ rejectIfNonZeroExitCode: true }));
   }, { pid: cp.pid, status: 1, signal: null });
 }
 
 
 {
   const fn = common.mustCall();
-  for await (const line of spawn(process.execPath, ['-e', 'console.error(42)']).readLines({ useStdErr: true })) {
+  for await (const line of spawn(process.execPath, ['-e', 'console.error(42)']).readLines({ listenTo: 'stderr' })) {
     assert.strictEqual(line, '42');
     fn();
   }
 }
 
 {
-  let stderr;
-  try {
-    execSync(`${process.execPath} -e 'throw new Error'`, { stdio: 'pipe', encoding: 'utf-8' });
-  } catch (err) {
-    if (!Array.isArray(err?.output)) throw err;
-    stderr = err.output[2].split(/\r?\n/);
-  }
+  const stderr = (await spawn(process.execPath, ['-e', 'throw new Error']).stderr.toArray()).join('').split(/\r?\n/);
   const cp = spawn(process.execPath, ['-e', 'throw new Error']);
-  for await (const line of cp.readLines({ useStdErr: true, ignoreErrors: true })) {
+  assert.strictEqual(cp.exitCode, null);
+  for await (const line of cp.readLines({ listenTo: 'stderr' })) {
     assert.strictEqual(line, stderr.shift());
   }
+  assert.strictEqual(cp.exitCode, 1);
   assert.deepStrictEqual(stderr, ['']);
 }