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

what's up waitpid + sleep loop in wait_or_timeout #6363

Open
mjguzik opened this issue May 5, 2024 · 1 comment
Open

what's up waitpid + sleep loop in wait_or_timeout #6363

mjguzik opened this issue May 5, 2024 · 1 comment

Comments

@mjguzik
Copy link

mjguzik commented May 5, 2024

I was stracing the timeout replacement and found it spams wait4 and clock_nanosleep calls.

Problematic code is:


    fn wait_or_timeout(&mut self, timeout: Duration) -> io::Result<Option<ExitStatus>> {
        if timeout == Duration::from_micros(0) {
            return self.wait().map(Some);
        }
        // .try_wait() doesn't drop stdin, so we do it manually
        drop(self.stdin.take());

        let start = Instant::now();
        loop {
            if let Some(status) = self.try_wait()? {
                return Ok(Some(status));
            }

            if start.elapsed() >= timeout {
                break;
            }

            // XXX: this is kinda gross, but it's cleaner than starting a thread just to wait
            //      (which was the previous solution).  We might want to use a different duration
            //      here as well
            thread::sleep(Duration::from_millis(100));
        }

        Ok(None)
    }

The original C variant gets away without doing anything of the sort.

Now, I'm not particularly good with rust but would be genuinely surprised if this was really necessary.

One clean solution would be to grab a file descriptor for the child (see the CLONE_PIDFD flag to clone) and one for timerfd. Then the code could nicely wait in an event loop.

I would implement this myself but I'm atrocious at Rust.

If stuff like the above kind be used, what's the reasoning?

While the comment above acknowledges the current state is not good, it does not justify it.

@mjguzik
Copy link
Author

mjguzik commented May 5, 2024

cc @Arcterus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants