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

3.1.0 switch to rustix again for glibc compat #32

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ libc = "0.2.152"
# Default enable napi4 feature, see https://nodejs.org/api/n-api.html#node-api-version-matrix
napi = { version = "2.12.2", default-features = false, features = ["napi4"] }
napi-derive = "2.12.2"
nix = { version = "0.29.0", features = ["fs", "term", "poll"] }
rustix = { version = "0.38.34", features = ["event", "termios"] }
rustix-openpty = "0.1.1"

[build-dependencies]
napi-build = "2.0.1"
Expand Down
2 changes: 1 addition & 1 deletion index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export class Pty {
* Returns a file descriptor for the PTY controller.
* See the docstring of the class for an usage example.
*/
fd(): c_int
fd(): RawFd
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think this is a legit type, ha. how can we make this return number?

/**
* 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!
Expand Down
45 changes: 21 additions & 24 deletions src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
use backoff::backoff::Backoff;
use backoff::ExponentialBackoffBuilder;
use libc::{self, c_int};
use napi::bindgen_prelude::JsFunction;
use napi::threadsafe_function::{ErrorStrategy, ThreadsafeFunction, ThreadsafeFunctionCallMode};
use napi::Status::GenericFailure;
use napi::{self, Env};
use nix::errno::Errno;
use nix::poll::{poll, PollFd, PollFlags, PollTimeout};
use nix::pty::{openpty, Winsize};
use nix::sys::termios::{self, SetArg};
use rustix::event::{poll, PollFd, PollFlags};
use rustix::io::Errno;
use rustix::termios::{self, InputModes, OptionalActions, Winsize};
use rustix_openpty::openpty;
use std::collections::HashMap;
use std::io::Error;
use std::io::ErrorKind;
use std::os::fd::{AsRawFd, OwnedFd};
use std::os::fd::{BorrowedFd, FromRawFd, RawFd};
use std::os::fd::{FromRawFd, RawFd};
use std::os::unix::process::CommandExt;
use std::process::{Command, Stdio};
use std::thread;
Expand Down Expand Up @@ -56,20 +55,18 @@ fn cast_to_napi_error(err: Errno) -> napi::Error {
// if the child process exits before the controller fd is fully read, we might accidentally
// end in a case where onExit is called but js hasn't had the chance to fully read the controller fd
// let's wait until the controller fd is fully read before we call onExit
fn poll_controller_fd_until_read(raw_fd: RawFd) {
fn poll_controller_fd_until_read(controller_fd: OwnedFd) {
// wait until fd is fully read (i.e. POLLIN no longer set)
let borrowed_fd = unsafe { BorrowedFd::borrow_raw(raw_fd) };
let poll_fd = PollFd::new(borrowed_fd, PollFlags::POLLIN);

let mut poll_fds = [PollFd::new(&controller_fd, PollFlags::IN)];
let mut backoff = ExponentialBackoffBuilder::default()
.with_initial_interval(Duration::from_millis(1))
.with_max_interval(Duration::from_millis(100))
.with_max_elapsed_time(Some(Duration::from_secs(1)))
.build();

loop {
if let Err(err) = poll(&mut [poll_fd], PollTimeout::ZERO) {
if err == Errno::EINTR || err == Errno::EAGAIN {
if let Err(err) = poll(&mut poll_fds, 0) {
if err == Errno::INTR || err == Errno::AGAIN {
// we were interrupted, so we should just try again
continue;
}
Expand All @@ -79,10 +76,9 @@ fn poll_controller_fd_until_read(raw_fd: RawFd) {
}

// check if POLLIN is no longer set (i.e. there is no more data to read)
if let Some(flags) = poll_fd.revents() {
if !flags.contains(PollFlags::POLLIN) {
break;
}
let flags = poll_fds[0].revents();
if !flags.contains(PollFlags::IN) {
break;
}

// wait for a bit before trying again
Expand Down Expand Up @@ -115,9 +111,9 @@ impl Pty {
}

// open pty pair
let pty_res = openpty(&window_size, None).map_err(cast_to_napi_error)?;
let controller_fd = pty_res.master;
let user_fd = pty_res.slave;
let pty_res = openpty(None, Some(&window_size)).map_err(cast_to_napi_error)?;
let controller_fd = pty_res.controller;
let user_fd = pty_res.user;

// duplicate pty user_fd to be the child's stdin, stdout, and stderr
cmd.stdin(Stdio::from(user_fd.try_clone()?));
Expand Down Expand Up @@ -160,8 +156,8 @@ impl Pty {
// set input modes
let user_fd = OwnedFd::from_raw_fd(raw_user_fd);
if let Ok(mut termios) = termios::tcgetattr(&user_fd) {
termios.input_flags |= termios::InputFlags::IUTF8;
termios::tcsetattr(&user_fd, SetArg::TCSANOW, &termios)?;
termios.input_modes.set(InputModes::IUTF8, true);
termios::tcsetattr(&user_fd, OptionalActions::Now, &termios)?;
}

// reset signal handlers
Expand Down Expand Up @@ -200,8 +196,9 @@ impl Pty {
thread::spawn(move || {
let wait_result = child.wait();

// try to wait for the controller fd to be fully read
poll_controller_fd_until_read(raw_controller_fd);
// try to wait for the controller fd to be fully read
let controller_fd = unsafe { OwnedFd::from_raw_fd(raw_controller_fd) };
poll_controller_fd_until_read(controller_fd);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i think this is unsafe, because we don't truly own the raw controller fd in this context, and exiting the poll will .close it! but then we will .close it again in .close()!!!

we probably need to reintroduce the fd copying that we had earlier 😭.


// we don't drop the controller fd immediately
// let pty.close() be responsible for closing it
Expand Down Expand Up @@ -272,7 +269,7 @@ impl Pty {
/// See the docstring of the class for an usage example.
#[napi]
#[allow(dead_code)]
pub fn fd(&mut self) -> Result<c_int, napi::Error> {
pub fn fd(&mut self) -> Result<RawFd, napi::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn fd(&mut self) -> Result<RawFd, napi::Error> {
pub fn fd(&mut self) -> Result<i32, napi::Error> {

it's disgusting, but i think we need to cast this to i32 so that the codegen emits a nice number https://napi.rs/docs/concepts/function

if let Some(fd) = &self.controller_fd {
Ok(fd.as_raw_fd())
} else {
Expand Down
Loading