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.

Sign up for GitHub

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

Jump to bottom

fix: work in progress macos support #8

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: work in progress macos support #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions src/pty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ impl Pty {
/// unable to put it into non-blocking mode.
pub fn new() -> crate::Result<Self> {
let pty = crate::sys::Pty::open()?;
pty.set_nonblocking()?;
Copy link
Author

Choose a reason for hiding this comment

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

Apparently, macos won't let ya do this prior to obtaining a pts.

Ok(Self(tokio::io::unix::AsyncFd::new(pty)?))
}

Expand All @@ -35,7 +34,9 @@ impl Pty {
/// Returns an error if the device node to open could not be determined,
/// or if the device node could not be opened.
pub fn pts(&self) -> crate::Result<Pts> {
Ok(Pts(self.0.get_ref().pts()?))
let pts = Pts(self.0.get_ref().pts()?);
self.0.get_ref().set_nonblocking()?;
Ok(pts)
Comment on lines -38 to +39
Copy link
Author

Choose a reason for hiding this comment

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

So we do it here. And we're not certain whether this is idempotent enough to not be an issue with multiple calls.

Copy link
Owner

Choose a reason for hiding this comment

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

i think this should be fine to call multiple times

}

/// Splits a `Pty` into a read half and a write half, which can be used to
Expand Down
14 changes: 6 additions & 8 deletions tests/basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ fn test_cat_blocking() {
use std::io::Write as _;

let mut pty = pty_process::blocking::Pty::new().unwrap();
let pts = pty.pts().unwrap();
pty.resize(pty_process::Size::new(24, 80)).unwrap();
let mut child = pty_process::blocking::Command::new("cat")
.spawn(&pty.pts().unwrap())
.spawn(&pts)
.unwrap();
Comment on lines +8 to 12
Copy link
Author

Choose a reason for hiding this comment

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

Similar to setting nonblocking, other operations also seem to be disallowed prior to obtaining a pts.


pty.write_all(b"foo\n").unwrap();
Expand All @@ -16,9 +17,7 @@ fn test_cat_blocking() {
assert_eq!(output.next().unwrap(), "foo\r\n");
assert_eq!(output.next().unwrap(), "foo\r\n");

pty.write_all(&[4u8]).unwrap();
let status = child.wait().unwrap();
assert_eq!(status.code().unwrap(), 0);
child.kill().unwrap();
Comment on lines -19 to +20
Copy link
Author

Choose a reason for hiding this comment

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

On macos, this ^D doesn't seem to kill the cat. 🤷 ??

If we care, we better figure out why. If we don't, we may as well allow the child to be dropped implicitly, I suppose.

Copy link
Owner

Choose a reason for hiding this comment

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

i'd rather figure out what's going on here - ^D does work to terminate a cat process when run normally on macos, so this crate (or test) is doing something wrong here, and i don't want to just sidestep that.

Copy link
Owner

Choose a reason for hiding this comment

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

i think this one is actually related to your change above - you're keeping the pts alive longer than before, which apparently prevents the wait from succeeding. if you instead do something like

    let mut child = {
        let pts = pty.pts().unwrap();
        pty.resize(pty_process::Size::new(24, 80)).unwrap();
        pty_process::blocking::Command::new("cat")
            .spawn(&pts)
            .unwrap()
    };

^D should work again.

Copy link
Author

Choose a reason for hiding this comment

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

Hrm... perhaps. Assuming this is true, wouldn't a user be prone to making a similar mistake such as we made in this test?

Copy link

Choose a reason for hiding this comment

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

When spawning a new process, the Pts is borrowed. Is this because it's meant for reuse? If not, can it instead by passed by value so that spawn() can drop it?

Copy link
Author

Choose a reason for hiding this comment

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

Pts is meant for reuse (probably in UNIX in general) as demonstrated in

pty-process/tests/behavior.rs

Lines 3 to 29 in 74c223d

#[test]
fn test_multiple() {
let pty = pty_process::blocking::Pty::new().unwrap();
let pts = pty.pts().unwrap();
pty.resize(pty_process::Size::new(24, 80)).unwrap();
let mut child = pty_process::blocking::Command::new("echo")
.arg("foo")
.spawn(&pts)
.unwrap();
let mut output = helpers::output(&pty);
assert_eq!(output.next().unwrap(), "foo\r\n");
let status = child.wait().unwrap();
assert_eq!(status.code().unwrap(), 0);
let mut child = pty_process::blocking::Command::new("echo")
.arg("bar")
.spawn(&pts)
.unwrap();
assert_eq!(output.next().unwrap(), "bar\r\n");
let status = child.wait().unwrap();
assert_eq!(status.code().unwrap(), 0);
}

So, alternatively, what does dropping the Pts do... or what does not dropping it do to prevent the cat child process from exiting on a signal?

Copy link
Author

Choose a reason for hiding this comment

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

It seems that blocking::Pts doesn't have a Drop impl and neither does sys::Pts. It would be std::os::fd::OwnedFd that needs to be dropped in order for cat to terminate on a signal. On macos, at least.

Which kind of makes me think that it may be that cat is terminating and something else does not. But that's just a hunch. We'd have to take a closer look. We can start with the wait method.

Copy link
Author

Choose a reason for hiding this comment

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

This library's spawn method returns a plain std::process::Child. So it is std::process::Child::wait that is blocking forever even though a signal was sent to cat.

One option: cat is not receiving the signal.
Another option: cat is receiving the signal and terminating—what else would it do?—but for another reason the wait method keeps blocking. Something that has to do with that non-dropped Pts.

}

#[cfg(feature = "async")]
Expand All @@ -40,9 +39,7 @@ async fn test_cat_async() {
assert_eq!(output.next().await.unwrap(), "foo\r\n");
assert_eq!(output.next().await.unwrap(), "foo\r\n");

pty_w.write_all(&[4u8]).await.unwrap();
let status = child.wait().await.unwrap();
assert_eq!(status.code().unwrap(), 0);
child.kill().await.unwrap()
Comment on lines -43 to +42
Copy link
Author

Choose a reason for hiding this comment

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

Similar to test_cat_blocking.

}

#[cfg(feature = "async")]
Expand All @@ -68,5 +65,6 @@ async fn test_yes_async() {
let bytes = pty_r.read_buf(&mut &mut buf[..]).await.unwrap();
assert_eq!(&buf[..bytes], b"y\r\n");

child.kill().await.unwrap()
#[cfg(not(target_os = "macos"))]
child.kill().await.unwrap();
Comment on lines -71 to +69
Copy link
Author

Choose a reason for hiding this comment

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

On macos this seems to hang. Why?

Copy link
Owner

Choose a reason for hiding this comment

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

similarly, this isn't doing anything obviously wrong, so i'd like to understand what is broken here before just disabling it.

}
Loading