Skip to content

Commit

Permalink
Fix a race between process death and stdio read
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
lhchavez committed May 5, 2024
1 parent 2aeef95 commit 61c93ac
Show file tree
Hide file tree
Showing 6 changed files with 412 additions and 130 deletions.
Binary file modified bun.lockb
Binary file not shown.
1 change: 1 addition & 0 deletions flake.nix
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
in
pkgs.mkShell {
buildInputs = with pkgs; [
nodejs_20
bun
cargo
libiconv
Expand Down
68 changes: 67 additions & 1 deletion index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,79 @@

/* auto-generated by NAPI-RS */

/** A size struct to pass to resize. */
export interface Size {
cols: number
rows: number
}
/**
* A very thin wrapper around PTYs and processes. The caller is responsible for calling `.close()`
* when all streams have been closed. We hold onto both ends of the PTY (controller and user) to
* prevent reads from erroring out with EIO.
*
* This is the recommended usage:
*
* ```
* const { Pty } = require('replit-ruspy');
* const fs = require('node:fs');
*
* const pty = new Pty('sh', [], ENV, CWD, { rows: 24, cols: 80 }, (...result) => {
* pty.close();
* // TODO: Handle process exit.
* });
*
* const read = new fs.createReadStream('', {
* fd: pty.fd(),
* start: 0,
* highWaterMark: 16 * 1024,
* autoClose: true,
* });
* const write = new fs.createWriteStream('', {
* fd: pty.fd(),
* autoClose: true,
* });
*
* read.on('data', (chunk) => {
* // TODO: Handle data.
* });
* read.on('error', (err) => {
* if (err.code && err.code.indexOf('EIO') !== -1) {
* // This is expected to happen when the process exits.
* return;
* }
* // TODO: Handle the error.
* });
* write.on('error', (err) => {
* if (err.code && err.code.indexOf('EIO') !== -1) {
* // This is expected to happen when the process exits.
* return;
* }
* // TODO: Handle the error.
* });
* ```
*/
export class Pty {
fd: number
/** The pid of the forked process. */
pid: number
constructor(command: string, args: Array<string>, envs: Record<string, string>, dir: string, size: Size, onExit: (err: null | Error, exitCode: number) => void)
/** Resize the terminal. */
resize(size: Size): void
/**
* Returns a file descriptor for the PTY controller. If running under node, it will dup the file
* descriptor, but under bun it will return the same file desciptor, since bun does not close
* the streams by itself. Maybe that is a bug in bun, so we should confirm the new behavior
* after we upgrade.
*
* See the docstring of the class for an usage example.
*/
fd(): c_int
/**
* Close the PTY file descriptor. This must be called when the readers / writers of the PTY have
* been closed, otherwise we will leak file descriptors!
*
* In an ideal world, this would be automatically called after the wait loop is done, but Node
* doesn't like that one bit, since it implies that the file is closed outside of the main
* event loop.
*/
close(): void
}
Loading

0 comments on commit 61c93ac

Please sign in to comment.