From 57f65f27b5672f47173fa722a1080375316efd05 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 26 Nov 2020 12:59:00 +0100 Subject: [PATCH 1/3] stdio: implement manual start for ReadStream All stdio ReadStream's use manual start to avoid consuming data for example when a process execs/spawns Refs: https://github.com/nodejs/node/issues/36251 --- .../bootstrap/switches/is_main_thread.js | 6 +++- lib/internal/streams/readable.js | 3 +- test/parallel/test-stdin-from-file-spawn.js | 34 +++++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-stdin-from-file-spawn.js diff --git a/lib/internal/bootstrap/switches/is_main_thread.js b/lib/internal/bootstrap/switches/is_main_thread.js index 08623898edafac..452031e1519f41 100644 --- a/lib/internal/bootstrap/switches/is_main_thread.js +++ b/lib/internal/bootstrap/switches/is_main_thread.js @@ -157,7 +157,11 @@ function getStdin() { case 'FILE': const fs = require('fs'); - stdin = new fs.ReadStream(null, { fd: fd, autoClose: false }); + stdin = new fs.ReadStream(null, { + fd: fd, + autoClose: false, + manualStart: true + }); break; case 'PIPE': diff --git a/lib/internal/streams/readable.js b/lib/internal/streams/readable.js index a004ce20d0aeb8..bed5aa73dde4d2 100644 --- a/lib/internal/streams/readable.js +++ b/lib/internal/streams/readable.js @@ -203,7 +203,8 @@ function Readable(options) { Stream.call(this, options); destroyImpl.construct(this, () => { - maybeReadMore(this, this._readableState); + if (!options || !options.manualStart) + maybeReadMore(this, this._readableState); }); } diff --git a/test/parallel/test-stdin-from-file-spawn.js b/test/parallel/test-stdin-from-file-spawn.js new file mode 100644 index 00000000000000..e27018fe856f95 --- /dev/null +++ b/test/parallel/test-stdin-from-file-spawn.js @@ -0,0 +1,34 @@ +'use strict'; +const common = require('../common'); +const process = require('process'); +if (process.platform !== 'linux' && process.platform !== 'darwin') + common.skip('This is an UNIX-only test'); + +const { execSync } = require('child_process'); +const fs = require('fs'); +const path = require('path'); +const tmpdir = require('../common/tmpdir'); + +const tmpDir = tmpdir.path; +tmpdir.refresh(); +const tmpCmdFile = path.join(tmpDir, 'test-stdin-from-file-spawn-cmd'); +const tmpJsFile = path.join(tmpDir, 'test-stdin-from-file-spawn.js'); +fs.writeFileSync(tmpCmdFile, 'echo hello'); +fs.writeFileSync(tmpJsFile, ` +'use strict'; +const { spawn } = require('child_process'); +// Reference the object to invoke the getter +process.stdin; +setTimeout(() => { + let ok = false; + const child = spawn(process.env.SHELL, [], { stdio: ['inherit', 'pipe'] }); + child.stdout.on('data', () => { + ok = true; + }); + child.on('close', () => { + process.exit(ok ? 0 : -1); + }); +}, 100); +`); + +execSync(`${process.argv[0]} ${tmpJsFile} < ${tmpCmdFile}`); From 33c1786a699078f7bb731f6685ea5b10a0093365 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Thu, 26 Nov 2020 14:40:34 +0100 Subject: [PATCH 2/3] stream: better platform compatibility --- test/parallel/test-stdin-from-file-spawn.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-stdin-from-file-spawn.js b/test/parallel/test-stdin-from-file-spawn.js index e27018fe856f95..12c235fcfeb081 100644 --- a/test/parallel/test-stdin-from-file-spawn.js +++ b/test/parallel/test-stdin-from-file-spawn.js @@ -1,8 +1,15 @@ 'use strict'; const common = require('../common'); const process = require('process'); -if (process.platform !== 'linux' && process.platform !== 'darwin') - common.skip('This is an UNIX-only test'); + +let defaultShell; +if (process.platform === 'linux' || process.platform === 'darwin') { + defaultShell = '/bin/sh'; +} else if (process.platform === 'win32') { + defaultShell = 'cmd.exe'; +} else { + common.skip('This is test exists only on Linux/Win32/OSX'); +} const { execSync } = require('child_process'); const fs = require('fs'); @@ -21,7 +28,8 @@ const { spawn } = require('child_process'); process.stdin; setTimeout(() => { let ok = false; - const child = spawn(process.env.SHELL, [], { stdio: ['inherit', 'pipe'] }); + const child = spawn(process.env.SHELL || '${defaultShell}', + [], { stdio: ['inherit', 'pipe'] }); child.stdout.on('data', () => { ok = true; }); From 5cd8cb6616d789ab8f15b63284dd0da07e685440 Mon Sep 17 00:00:00 2001 From: Momtchil Momtchev Date: Tue, 5 Jan 2021 13:38:18 +0100 Subject: [PATCH 3/3] add an unit test with the stream API --- test/parallel/test-stream-manualstart.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 test/parallel/test-stream-manualstart.js diff --git a/test/parallel/test-stream-manualstart.js b/test/parallel/test-stream-manualstart.js new file mode 100644 index 00000000000000..2c7442a5538db8 --- /dev/null +++ b/test/parallel/test-stream-manualstart.js @@ -0,0 +1,20 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs'); + +fs.promises.open(__filename).then(common.mustCall((fd) => { + const rs = new fs.ReadStream(null, { + fd: fd, + manualStart: false + }); + setTimeout(() => assert(rs.bytesRead > 0), common.platformTimeout(10)); +})); + +fs.promises.open(__filename).then(common.mustCall((fd) => { + const rs = new fs.ReadStream(null, { + fd: fd, + manualStart: true + }); + setTimeout(() => assert(rs.bytesRead === 0), common.platformTimeout(10)); +}));