assert-rs / assert_cmd

Assert process::Command - Easy command initialization and assertions
https://docs.rs/assert_cmd
Apache License 2.0
463 stars 34 forks source link

.timeout() functionality does not work on payload processes with children #167

Open mrkmndz opened 1 year ago

mrkmndz commented 1 year ago

The implementation of timeout implemented in #92 does not work as intended for payload processes with children.

Namely, it will get stuck here:

        let stdout = stdout
            .and_then(|t| t.join().unwrap().ok())
            .unwrap_or_default();

waiting for the stdout to EOF, while the "grandchild" process happily keeps that fd open indefinitely.

In order to behave as expected, we need to explicitly kill all transitive children of the payload process here:

                .unwrap_or_else(|| {
                    let _ = child.kill();
                    child.wait()
                })

or at least apply some sort of timeout logic on the read threads.

cc @epage

epage commented 1 year ago

What OS is this on and can you create a reproduction case to add to the tests?

When the child process is killed, I would expect the OS to close the stdio which would cause the reader threads to end

mrkmndz commented 1 year ago

The file descriptor will be closed if there are no more processes attached to it, but if the initial payload process spawns a child process before being killed, and that child process inherits the standard out of the payload process, then after the kill there will still be a process attached to the stdout fd, so it will not be closed.

I can try to put up a test for this, but I probably won't get to it for a while.

epage commented 1 year ago

If we cancel the read threads, what would be keeping us from leaking the child processes? I feel like the main issue here is that those aren't being killed and not us continuing to read from them.