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

spawnSync()'s maxBuffer option is not ignored with stdio[3]: 'ignore' #52338

Closed
ehmicky opened this issue Apr 3, 2024 · 4 comments
Closed

spawnSync()'s maxBuffer option is not ignored with stdio[3]: 'ignore' #52338

ehmicky opened this issue Apr 3, 2024 · 4 comments
Labels
child_process Issues and PRs related to the child_process subsystem.

Comments

@ehmicky
Copy link

ehmicky commented Apr 3, 2024

Version

v21.7.1

Platform

Linux my-laptop 6.5.0-26-generic #26-Ubuntu SMP PREEMPT_DYNAMIC Tue Mar 5 21:19:28 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

child_process

What steps will reproduce the bug?

With print.js:

import {writeSync} from 'node:fs'

const fdNumber = Number(process.argv[2])
writeSync(fdNumber, 'oo') // 2 bytes, above the `maxBuffer: 1` threshold below

Then example.js:

import {spawnSync} from 'node:child_process'

const {error, output} = spawnSync('node', ['./print.js', '3'], {
  maxBuffer: 1, 
  stdio: ['pipe', 'pipe', 'pipe', 'ignore'],
})
console.log(error) // ENOBUFS
console.log(output[3]) // null

How often does it reproduce? Is there a required condition?

No.

What is the expected behavior? Why is that the expected behavior?

Since stdio[3] is ignore, the maxBuffer option should not apply to it. I.e. the ENOBUFS error should not be present. For example, this is what happens with stdio[2].

const {error, output} = spawnSync('node', ['./print.js', '2'], {
  maxBuffer: 1, 
  stdio: ['pipe', 'pipe', 'ignore'],
})
console.log(error) // undefined
console.log(output[2]) // null

What do you see instead?

The ENOBUFS error is present.

Additional information

No response

@kylo5aby
Copy link
Contributor

kylo5aby commented Apr 3, 2024

IMO the correct behavior is

console.log(error) // ENOBUFS
console.log(output[3]) // null

instead of

console.log(error) // undefined
console.log(output[2]) // null

because ENOBUFS refers to any errors encountered during the execution of spawnSync instead of subprocess.

@ehmicky
Copy link
Author

ehmicky commented Apr 3, 2024

I think there are two separate questions. The first is: should the maxBuffer option apply to file descriptors which use 'ignore'? If 'ignore' is used, it is implied that the output of that file descriptor is discarded. Therefore, there is no buffering, and the maxBuffer option should not apply. Therefore, spawnSync() should not encounter any ENOBUFS error.

The second thing is: regardless of what the correct behavior is there, stdio[2] and stdio[3] should behave the same way. Currently, stdio[2]: 'ignore' does not apply maxBuffer, but stdio[3]: 'ignore' does. This is not consistent.

@VoltrexKeyva VoltrexKeyva added the child_process Issues and PRs related to the child_process subsystem. label Apr 3, 2024
@kylo5aby
Copy link
Contributor

kylo5aby commented Apr 8, 2024

I agree with you. I have run the case What steps will reproduce the bug, and it can be reproduced. however, on my computer, stderr outputted Error: EINVAL: invalid argument, write,

import {spawnSync} from 'node:child_process'
let {error, output} = spawnSync('node', ['./print.mjs', '3'], {
  maxBuffer: 1,
  stdio: ['pipe', 'pipe', 'pipe', 'ignore'],
})
// console.log(error) // ENOBUFS
console.log(output[2].toString()) // Error: EINVAL: invalid argument, write

so I believe that for ignored file descriptors, besides stdin, stdout, and stderr, all are invalid. Therefore, in this case, this error was inputted to stderr of the pipe, in other words, the outputted Error ENOBUFS comes from stderr.

@ehmicky
Copy link
Author

ehmicky commented Apr 8, 2024

You're correct.

So actually there is no inconsistency with maxBuffer, but with ignore instead.
It is also not specific to spawnSync(), and happens with spawn() too.

I have opened #52422 to continue the discussion, since this is rather different from what I initially reported.

@ehmicky ehmicky closed this as completed Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants