Prevent external processes from hanging forever

[CrispyDrone]

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Bug fix.

⤵️ What is the current behavior?

In certain cases, invoking an external process will result in NuKeeper hanging forever waiting for it to terminate. This can reliably be reproduced (in my case), when debugging NuKeeper. When invoking NuKeeper from the command line (without a debugger attached?), this does not happen.

💥 Does this PR introduce a breaking change?

No.

🐛 Recommendations for testing

Try debugging NuKeeper. For me this consistently hangs on the first restore solution command. This also happens when running it from bash. It does not happen when I run from the windows cmd console (perhaps after invoking VsDevCmd.bat, since I've been doing that to prevent other problems).

📝 Links to relevant issues/docs

• NuKeeper hangs on NuGet restore command #1004

🤔 Checklist before submitting

No relevant documentation, I think.

• All projects build
• Relevant documentation was updated

[scp-mb]

Ran into this issue today when we had some unit test projects that failed to restore packages, so stdout was written to first, which would cause our nukeeper CI pipeline to hang until the 1 hour timeout.

[skolima]

I've intermittently seen the error myself (though never managed to reproduce), but I'm confused how this change would fix it. Task.WhenAll requires tasks to finish without regard to the ordering, but still requires both of them to finish. How does this resolve the issue of hanged behaviour?

And could you please include a test for this change?

[Greybird]

I've intermittently seen the error myself (though never managed to reproduce), but I'm confused how this change would fix it. Task.WhenAll requires tasks to finish without regard to the ordering, but still requires both of them to finish. How does this resolve the issue of hanged behaviour?

I think the key thing might be that both tasks will be started before waiting for one to complete.

The previous code is waiting to read all the output stream, then waits for the error stream to be completely read.
These streams have finite size, and the child executable could have filled the error stream, waiting for it be have some room to write more.
However, as the parent process will be waiting to have finished reading the output stream (which is not the case as the child process could write to it afterwards), before reading the error stream.
Result is that both processes are deadlocked.

You can find some details about these caveats of these streams in the ms doc, where in the last example, the error stream is read using the asynchronous method (ie events) while the output is read synchronoulsy (these synchronous/asynchronous not to be confused with async/await usage)

[skolima]

This (and the MSDN docs) make sense now. Thanks.