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

Add race condition-related tests #618

Merged
merged 1 commit into from
Dec 17, 2023
Merged

Add race condition-related tests #618

merged 1 commit into from
Dec 17, 2023

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Dec 17, 2023

Fixes #616.

This adds some tests related to the race condition that happens when we redirect a child process's stdin/stdout/stderr right after spawning it.

Since it's been just spawned, it might already have read/written from/to its standard streams, before we get a chance to pipe from/to them.

However, things work mostly correctly thanks to proper behavior from both the OS and the runtime, as described in the end of my comment here.

Unfortunately, there is one of those assumptions that does not seem to hold true. Namely, a process's stdout/stderr might be lost if either:

  • It already exited before await childProcess was called. This is most likely to happen for users that do not call that statement right away.
  • It already exited before we pipe its stdout/stderr (when using the stdio: filePath | fileURL | WritableStream option). This might happen if the child process is very fast. This includes when the child process fails very fast, although in that case the user gets the exit code at least, but might be missing some precious error logs.

I am not sure there is something we can do about this. At the OS-level, the process spawning syscalls just sets up the standard streams. Reading/writing them must happen after spawning. And the process might have already exited. I do not think this is not specific to Node.js.

@ehmicky ehmicky force-pushed the test-race-condition branch 2 times, most recently from 7398a77 to 53d1f2a Compare December 17, 2023 19:09
@sindresorhus sindresorhus merged commit bea94d6 into main Dec 17, 2023
14 checks passed
@sindresorhus sindresorhus deleted the test-race-condition branch December 17, 2023 20:16
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 this pull request may close these issues.

Timing problem when piping process.stdin|stdout|stderr
2 participants