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

Missing stdout due to closed stream #40

Closed
AriPerkkio opened this issue Dec 4, 2024 · 1 comment · Fixed by #41
Closed

Missing stdout due to closed stream #40

AriPerkkio opened this issue Dec 4, 2024 · 1 comment · Fixed by #41

Comments

@AriPerkkio
Copy link
Member

I ran into edge case while working on Vitest. Unfortunately I was unable to create minimal reproduction. Reproduction with Vitest: AriPerkkio/vitest@de3dc71

When we add exit listener that calls process.exit inside it, Tinyexec loses stdout from that process. This is similar exit handling as tinylibs/picospinner does.

It seems that subprocess.stdout returned from node:child_process.spawn is closed when subprocess.stderr is read. Here's some logging from Vitest's case:

tinyexec/src/main.ts

Lines 216 to 226 in 24e9861

if (this._streamErr) {
for await (const chunk of this._streamErr) {
stderr += chunk.toString();
}
}
if (this._streamOut) {
for await (const chunk of this._streamOut) {
stdout += chunk.toString();
}
}

+ console.log('before', this._streamOut?.closed);
+ // > before false

if (this._streamErr) {
  for await (const chunk of this._streamErr) {
    stderr += chunk.toString();
  }
}

+ console.log('after', this._streamOut?.closed);
+ // > after true

if (this._streamOut) {
  for await (const chunk of this._streamOut) {
    stdout += chunk.toString();
  }
}

To fix these, we'll need to first read stdout.

Replacing tinyexec with node:child_process.execSync works 100% times.

@ghiscoding
Copy link

@AriPerkkio @43081j not sure if it's related at all, but I saw this recent NodeJS PR today worker: flush stdout and stderr on exit

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