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

Tests in panic=abort can hang if test spawns a subprocess #68936

Open
tmandry opened this issue Feb 7, 2020 · 3 comments
Open

Tests in panic=abort can hang if test spawns a subprocess #68936

tmandry opened this issue Feb 7, 2020 · 3 comments
Labels
A-libtest Area: `#[test]` / the `test` library C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Feb 7, 2020

If a test (running in a subprocess, because panic=abort) spawns a sub-subprocess which inherits its stdout or stderr handles, and the sub-subprocess does not exit when the test does, the test framework will hang. This is because wait_with_output waits for all stdio handles to be closed.

libtest runner         (waits for unit test stdio to close)
       |
       |  Stdio::piped
       |
unit test process      (exits, but stdio is now shared with subproc)
      ||
      ||  Stdio::inherit
      ||
"leaked" subprocess    (sticks around forever)

Some tests spawn subprocesses, and it can be hard (particularly with panic=abort) to ensure that they are all cleaned up when the test succeeds or fails. In these cases, it would be best not to hang. Hanging is unexpected, and hard to debug.

@tmandry
Copy link
Member Author

tmandry commented Feb 7, 2020

IMO, the ideal semantics in this case would be for a test's subprocesses to automatically die when the test exits, but that's different from the semantics of std::process::Child. Changing semantics only in test mode is generally a bad idea.

Failing that, we should avoid hanging and/or give a more helpful error message when this situation arises.

Not hanging can be a pain to implement on unix: we'd have to continue reading from all stdio handles, to avoid deadlock, but also watching for the process to exit without the handles being closed. That means installing a signal handler for SIGCHLD.(not necessarily, see below) On Fuchsia it is easier, but we couldn't share the implementation with unix. For Windows, I'm not sure.

Finally, it isn't clear whether this is the "correct" behavior for wait_with_output, or whether that can be changed. We might need to introduce a new method, perhaps in libstd behind a feature flag.

@jonas-schievink jonas-schievink added A-libtest Area: `#[test]` / the `test` library C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 7, 2020
@tmandry
Copy link
Member Author

tmandry commented Mar 12, 2020

Update: I don't see a way of this getting fixed without async I/O in libtest.

Unless we want to spawn an extra 2 threads per test (one each for reading stdout/stderr), which seems bad for performance.

@tmandry
Copy link
Member Author

tmandry commented Aug 14, 2020

I suppose this is possible to do on generic POSIX with something like the following:

Spawn one thread to read the file handles, and have another to wait on the subprocess with wait/waitpid. Once the subprocess dies, signal the first thread through some mechanism (closing the file handles? a third file handle?).

On Fuchsia the extra thread isn't needed, and I'm still unsure how this affects Windows.

@Enselic Enselic added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-libtest Area: `#[test]` / the `test` library C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
Status: No status
Development

No branches or pull requests

3 participants