Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stdio: implement manual start for ReadStream #36277

Closed
wants to merge 3 commits into from
Closed

stdio: implement manual start for ReadStream #36277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57f65f2
stdio: implement manual start for ReadStream
mmomtchev Nov 26, 2020
33c1786
stream: better platform compatibility
mmomtchev Nov 26, 2020
5cd8cb6
add an unit test with the stream API
mmomtchev Jan 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/internal/bootstrap/switches/is_main_thread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
ronag marked this conversation as resolved.
Show resolved Hide resolved
manualStart: true
});
break;

case 'PIPE':
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/streams/readable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,8 @@ function Readable(options) {
Stream.call(this, options);

destroyImpl.construct(this, () => {
maybeReadMore(this, this._readableState);
if (!options || !options.manualStart)
ronag marked this conversation as resolved.
Show resolved Hide resolved
maybeReadMore(this, this._readableState);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs some tests that are specific to streams. How is a user of Stream be able to use this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina @joyeecheung @ronag @mscdex

Attaching a data handler will start the stream.
I simply recreated the semantics of the non-documented option in net.Socket - which is what the thread bootstrap code uses.
Its only user at the moment is this thread bootstrap code for the stdin stream. The net.Socket option has a second use case in TLSWrap - but I am not sure if it actually works - because this wouldn't happen : #35475

The reason I did in Readable is simply that the reading is started from here for all Readable except net.Socket which has its own startup code

To move this to ReadStream I would need to reimplement part of this code and it will be a much more invasive change

So two options:

  • Keep both manualStart options as hidden non-documented options, eventually replacing them with Symbol names
  • Document them, unit test them and make them official

It is also worth noting that there are two other net.Socket specific options, both non-documented, pauseOnStart and pauseOnConnect - which have different semantics - the bootstrap code being made for manualStart

ronag marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down
42 changes: 42 additions & 0 deletions test/parallel/test-stdin-from-file-spawn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
'use strict';
const common = require('../common');
const process = require('process');

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');
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 || '${defaultShell}',
[], { stdio: ['inherit', 'pipe'] });
child.stdout.on('data', () => {
ok = true;
});
child.on('close', () => {
process.exit(ok ? 0 : -1);
});
}, 100);
`);

execSync(`${process.argv[0]} ${tmpJsFile} < ${tmpCmdFile}`);
20 changes: 20 additions & 0 deletions test/parallel/test-stream-manualstart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you using fs.promises? Could you please use fs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because a cook who doesn't eat its own food is not a good cook 😄

const rs = new fs.ReadStream(null, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fs.ReadStream, not stream.Readable. While this test is correct, can you please use one that just use stream.Readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Readable always starts in paused mode, this change affects only a ReadStream

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Readable always starts in paused mode

How is that relevant? It behaves the same as ReadStream. The same test should apply.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Readable always starts in paused mode, this change affects only a ReadStream

If that was true, you'd not need to modify Readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there is some biggus problem, how do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there is some biggus problem, how do you think?

Likely, but it's possible to add an independent test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused what the problem here is? We just need change ReadStream to Readable. What is the "biggus" problem?

fd: fd,
manualStart: false
});
setTimeout(() => assert(rs.bytesRead > 0), common.platformTimeout(10));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be a setImmediate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The read handler is already in a setImmediate - so this will run the assert before Node has a chance to start reading data.
I don't have any ideas on how to achieve this without a timer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the clarification. Maybe add this in a comment?

}));

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

his should be a setImmediate

}));