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

Avoid the Bun 1.1.7 bug #9

Merged
merged 4 commits into from
May 6, 2024
Merged

Avoid the Bun 1.1.7 bug #9

merged 4 commits into from
May 6, 2024

Conversation

lhchavez
Copy link
Contributor

@lhchavez lhchavez commented May 5, 2024

Bun currently has a bug where raw FDs don't interact well with fs.createReadStream, and the result is that not all data is seen by the 'data' callback: oven-sh/bun#9907

This change is an egregious workaround that makes Rust read the file until the process closes it. It is suboptimal because of all the data copying that happens, and crossing the FFI barrier also takes some time, but at least this should let us not be completely blocked. Now there is one more (optional) argument to Pty that is a replacement of the 'data' callback.

~/ruspty$ cat test.js 
const { Pty } = require('./index');
const fs = require('fs');

const ENV = process.env;
const CWD = '.';

let firstread = true;

const pty = new Pty(
  'sh',
  [],
  ENV,
  CWD,
  { rows: 24, cols: 80 },
  (...result) => {
    console.log({ result });
    pty.close();
  },
  (err, chunk) => {
    if (err !== null) {
      throw err;
    }
    console.log(chunk.toString());
    if (firstread) {
      write.write('ls\n', () => {
        console.log('ls written');
        write.end('exit\n', () => {
          console.log('end done');
        });
      });
      firstread = false;
    }
  },
);

const write = fs.createWriteStream('', {
  fd: pty.fd(),
  autoClose: true,
});

write.on('close', () => {
  console.log('write close');
});

write.on('error', (err) => {
  if (err.code && err.code.indexOf('EIO') !== -1) {
    console.log('YAY');
  } else {
    console.log('write error', { err });
  }
});

console.log('gonna wait');
setTimeout(() => {
  console.log('done, hopefully');
}, 3000);
~/ruspty$ ./node_modules/.bin/bun --version 
1.1.7
~/ruspty$ ./node_modules/.bin/bun test.js 
gonna wait
sh-5.2$ 
ls written
end done
ls

exit

write close
1.0.26.txt  build.rs    index.js       package-lock.json          src
1.1.7.txt   bun.lockb   index.test.ts  package.json               target
Cargo.lock  flake.lock  node.txt       replit.nix                 test.js
Cargo.toml  flake.nix   node_modules   ruspty.linux-x64-gnu.node
README.md   index.d.ts  npm            rustfmt.toml

sh-5.2$ 
exit

{
  result: [ null, 0 ],
}
done, hopefully

@lhchavez lhchavez force-pushed the lh-avoid-the-bun-bug branch 3 times, most recently from 9187411 to 82cb52d Compare May 5, 2024 17:03
@lhchavez lhchavez force-pushed the lh-fix-race-between-process-death-and-stdio-read branch from 2479a55 to ecdec1c Compare May 5, 2024 17:14
@lhchavez lhchavez force-pushed the lh-avoid-the-bun-bug branch from 82cb52d to 9d1e27f Compare May 5, 2024 17:14
@lhchavez lhchavez force-pushed the lh-fix-race-between-process-death-and-stdio-read branch 2 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 from 38b52f3 to 61c93ac Compare May 5, 2024 17:38
lhchavez added 2 commits May 5, 2024 18:04
The Pty constructor is going to grow soon, so might as well make this
refactor now.

This change makes some of the parameters to Pty optional for ergonomics.
Bun currently has a bug where raw FDs don't interact well with
`fs.createReadStream`, and the result is that not all data is seen by
the `'data'` callback: oven-sh/bun#9907

This change is an egregious workaround that makes Rust read the file
until the process closes it. It is suboptimal because of all the data
copying that happens, and crossing the FFI barrier also takes some time,
but at least this should let us not be completely blocked. Now there is
one more (optional) argument to `Pty` that is a replacement of the
`'data'` callback.

This unfortunately is Linux-only.
@lhchavez lhchavez force-pushed the lh-avoid-the-bun-bug branch from 9d1e27f to 86efdc1 Compare May 5, 2024 18:05
@lhchavez lhchavez changed the base branch from lh-fix-race-between-process-death-and-stdio-read to lh-pty-ctor-object May 5, 2024 18:05
Base automatically changed from lh-pty-ctor-object to main May 6, 2024 08:36
@szymonkaliski szymonkaliski enabled auto-merge (squash) May 6, 2024 09:14
@szymonkaliski szymonkaliski merged commit d83a57a into main May 6, 2024
5 checks passed
@szymonkaliski szymonkaliski deleted the lh-avoid-the-bun-bug branch May 6, 2024 09:14
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