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

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

Open
mrkmndz opened this issue Mar 23, 2023 · 3 comments
Open

Comments

@mrkmndz
Copy link

mrkmndz commented Mar 23, 2023

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
Copy link
Contributor

epage commented Mar 23, 2023

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
Copy link
Author

mrkmndz commented Mar 23, 2023

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
Copy link
Contributor

epage commented Mar 24, 2023

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.

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

No branches or pull requests

2 participants