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.

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

Remove file descriptor restriction on the stdin/stdout/stderr option #617

Closed
ehmicky opened this issue Dec 17, 2023 · 3 comments · Fixed by #644
Closed

Remove file descriptor restriction on the stdin/stdout/stderr option #617

ehmicky opened this issue Dec 17, 2023 · 3 comments · Fixed by #644

Comments

@ehmicky
Copy link
Collaborator

ehmicky commented Dec 17, 2023

Node.js requires the streams passed to the stdio option to have an underlying file descriptor.

https://github.com/nodejs/node/blob/0afe731d357d4df9a65aeb06429bf663d8dc2975/lib/internal/child_process.js#L1060
https://github.com/nodejs/node/blob/0afe731d357d4df9a65aeb06429bf663d8dc2975/lib/internal/child_process.js#L340

Execa could remove that restriction: if a stream without a file descriptor is passed to the stdio option(s), we could pipe it to the child process after it spawned. This would re-use the code that does the same thing for web streams (#615).

This was actually already discussed in #81, although in different terms.

However, Node.js detects whether there is an underlying file descriptor using the undocumented stream._handle and stream.handle properties. Also, those must be instances of Pipe, TTY, TCP or UDP which are internal classes not accessible userland. I do not think we should build detection logic based on this, as any Node.js release might break it.

Alternatively, we could make users opt-in to that behavior. For example, wrapping the stream into an array or a plain object instead of passing stream directly, to the stdio option(s).

Or, users could turn the Node.js stream into a web stream which we now support (#603), using Readable.toWeb()/Writable.toWeb(). I.e. we would just add some documentation about this.

What are your thoughts on this @sindresorhus?

@ehmicky ehmicky changed the title Remove file description restriction on the stdin/stdout/stderr option Remove file descriptor restriction on the stdin/stdout/stderr option Dec 17, 2023
@sindresorhus
Copy link
Owner

I prefer to put minimal effort into legacy APIs like Node.js streams. So my preference would be documentation:

Or, users could turn the Node.js stream into a web stream which we now support (#603), using Readable.toWeb()/Writable.toWeb(). I.e. we would just add some documentation about this.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Dec 18, 2023

Yes, I was also thinking along those lines, for the same reason. 👍

@ehmicky
Copy link
Collaborator Author

ehmicky commented Dec 27, 2023

When adding support for passing multiple values to the std* options (#643), we added a possible workaround for this issue, as a side effect. Namely, when passing a Node.js stream + another value, the Node.js stream will be handled by Execa instead of by child_process.spawn(). This removes the restriction above.

For example, { stdout: [nodeStream, 'pipe'] } instead of { stdout: nodeStream }.

We can document this workaround as it might help some users (done at #644).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants