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

Fix a race between process death and stdio read #8

Merged

Conversation

lhchavez
Copy link
Contributor

@lhchavez lhchavez commented May 5, 2024

Previously we used to close the fd immediately after the process was terminated. That made lifetimes simple to reason about, but we accidentally introduced a very subtle bug: If bun hadn't read the entirety of the stdout/stderr of the child process, since Rust would close the file from under it, Bun would not be able to read the rest of the output, truncating it!

This change now adds an explicit close() and fd() functions so that the fd management becomes way more explicit than before. Tests were also updated to ensure that there are no leaked FDs, since now we lost the guarantee that everything was going to be cleaned up properly.

~/ruspty$ bun run build && bun test
$ napi build --platform --release
   Compiling replit_ruspty v1.0.0 (/home/lhchavez/ruspty)
    Finished release [optimized] target(s) in 15.19s
bun test v1.0.26 (c75e768a)

index.test.ts:
✓ PTY > spawns and exits [11.96ms]
✓ PTY > captures an exit code [7.81ms]
✓ PTY > can be written to [10.44ms]
✓ PTY > can be resized [9.45ms]
✓ PTY > respects working directory [7.24ms]
✓ PTY > respects env [110.81ms]
✓ PTY > works with Bun.read & Bun.write [15.14ms]
✓ PTY > doesn't break when executing non-existing binary [3.73ms]

 8 pass
 0 fail
 11 expect() calls
Ran 8 tests across 1 files. [334.00ms]

@lhchavez lhchavez force-pushed the lh-fix-race-between-process-death-and-stdio-read branch 4 times, most recently from 7bb1794 to 38b52f3 Compare May 5, 2024 17:34
Previously we used to close the fd immediately after the process was
terminated. That made lifetimes simple to reason about, but we
accidentally introduced a very subtle bug: If bun hadn't read the
entirety of the stdout/stderr of the child process, since Rust would
close the file from under it, Bun would not be able to read the rest of
the output, truncating it!

This change now adds an explicit `close()` and `fd()` functions so that
the fd management becomes way more explicit than before. Tests were also
updated to ensure that there are no leaked FDs, since now we lost the
guarantee that everything was going to be cleaned up properly.
@lhchavez lhchavez force-pushed the lh-fix-race-between-process-death-and-stdio-read branch 2 times, most recently from 8ce8e61 to 61c93ac Compare May 5, 2024 18:05
@szymonkaliski szymonkaliski merged commit 06c940a into main May 6, 2024
5 checks passed
@szymonkaliski szymonkaliski deleted the lh-fix-race-between-process-death-and-stdio-read branch May 6, 2024 08:27
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.

2 participants