portable-pty windows failed to write to pty

[sigmaSd]

What Operating System(s) are you seeing this problem on?

Windows

Which Wayland compositor or X11 Window manager(s) are you using?

No response

WezTerm version

wezterm 20230712-072601-f4abf8fd

Did you try the latest nightly build to see if the issue is better (or worse!) than your current version?

No, and I'll explain why below

Describe the bug

windows fails to write to the pty, if the pair.slave is dropped

To Reproduce

use portable_pty::{CommandBuilder, NativePtySystem, PtySize, PtySystem};
use std::sync::mpsc::channel;

fn main() {
    let pty_system = NativePtySystem::default();

    let pair = pty_system
        .openpty(PtySize {
            rows: 24,
            cols: 80,
            pixel_width: 0,
            pixel_height: 0,
        })
        .unwrap();

    dbg!(std::process::Command::new("deno").arg("--version").spawn());
    let mut cmd = if cfg!(windows) {
        let mut cmd = CommandBuilder::new("deno");
        cmd.env("PATH", std::env::var("PATH").unwrap());
        // // cmd.args(["/C", "deno"]);
        // // let mut cmd = CommandBuilder::new(r"C:\hostedtoolcache\windows\deno\1.36.3\x64\deno.exe");
        // cmd.args(["-Command", "deno"]);
        cmd
    } else {
        CommandBuilder::new("deno")
    };
    cmd.env("NO_COLOR", "1");
    let _child = pair.slave.spawn_command(cmd).unwrap();

    // Release any handles owned by the slave: we don't need it now
    // that we've spawned the child.
    if !cfg!(windows) { 
        drop(pair.slave);
    }

    // Read the output in another thread.
    // This is important because it is easy to encounter a situation
    // where read/write buffers fill and block either your process
    // or the spawned process.
    let (tx, rx) = channel();
    let mut reader = pair.master.try_clone_reader().unwrap();
    std::thread::spawn(move || {
        loop {
            // Consume the output from the child
            let mut s = [0; 512];
            let n = reader.read(&mut s).unwrap();
            tx.send(String::from_utf8_lossy(&s[..n]).to_string())
                .unwrap();
        }
    });

    {
        // Obtain the writer.
        // When the writer is dropped, EOF will be sent to
        // the program that was spawned.
        // It is important to take the writer even if you don't
        // send anything to its stdin so that EOF can be
        // generated, otherwise you risk deadlocking yourself.
        let mut writer = pair.master.take_writer().unwrap();

        if cfg!(target_os = "macos") {
            // macOS quirk: the child and reader must be started and
            // allowed a brief grace period to run before we allow
            // the writer to drop. Otherwise, the data we send to
            // the kernel to trigger EOF is interleaved with the
            // data read by the reader! WTF!?
            // This appears to be a race condition for very short
            // lived processes on macOS.
            // I'd love to find a more deterministic solution to
            // this than sleeping.
            std::thread::sleep(std::time::Duration::from_millis(20));
        }

        // This example doesn't need to write anything, but if you
        // want to send data to the child, you'd set `to_write` to
        // that data and do it like this:
        let to_write = "5+4\n\r";
        if !to_write.is_empty() {
            // To avoid deadlock, wrt. reading and waiting, we send
            // data to the stdin of the child in a different thread.
            std::thread::spawn(move || {
                writer.write_all(to_write.as_bytes()).unwrap();
                dbg!(4);
            });
        }
    }

    // Take care to drop the master after our processes are
    // done, as some platforms get unhappy if it is dropped
    // sooner than that.
    drop(pair.master);

    // Now wait for the output to be read by our reader thread
    for _ in 0..5 {
        dbg!(rx.recv().unwrap());
        std::thread::sleep_ms(100);
    }
}

the important part is

  if !cfg!(windows) { 
        drop(pair.slave);
    }

if the slave is dropped, windows error when trying to write here writer.write_all(to_write.as_bytes()).unwrap();

[wez]

That example is a somewhat contrived single-use example for whoami. A real program will probably keep the slave side around for longer, and manage the lifetime of the various parts differently. I think you might want to try moving the drop after the write, for example. Or alternatively, introduce a sleep similar to the macos quirk, so that process(es) being spawned into the pty by the launched process have time to start up and so stuff.

I don't see this as a bug in portable-pty, but rather a documentation issue: this sounds like another instance of a multi-process data-race style scenario that is common when dealing with child process spawning and coordinating input/output around them.

[sigmaSd]

I think just adding some inline docs to the pty examples is enough

[sigmaSd]

Storing the slave, makes pty.read hangs if called after the process exit.

This is the workaround I got with help from wez.

        // If we do a pty.read after the process exit, read will hang
        // Thats why we spawn another thread to wait for the child
        // and signal its exit
        let tx_read_c = tx_read.clone();
        std::thread::spawn(move || {
            let _ = child.wait();
            let _ = tx_read_c.send(Message::End);
        });

I think the best thing is to add a new example that shows how to handle all theses cases.

[sigmaSd]

Another way to do this is , instead of storing the slave, drop the salve immediately after the spawn, but only drop the master after the child process exits, example here https://github.com/nrwl/nx/blob/202f14177833e021f86d646969fd2e2fee8dbb97/packages/nx/src/native/command.rs#L129-L217