ProcessRunner: do not call WaitForExit until redirection has started

[mslukebo]

If Process.WaitForExit is called before BeginOutputReadLine and/or BeginErrorReadLine is called, WaitForExit will not wait until the end of their streams are redirected. This results in a race condition where, if too much time elapses between Process.Start and BeginOutputReadLine/BeginErrorReadLine, it's possible to not receive any output before the process exits and the TaskCompletionSource gets its result.

This is particularly problematic since slower build machines that use this tool may randomly see the tool failing due to it not finding the dotnet CLI version, since the version is read from a dotnet --version process output redirection.

[mslukebo]

This bug has personally been affecting my builds that run against .NET 6. Assuming this fix is accepted, is there any chance it can be back-ported to that SDK?

[sharwell]

@AArnott this seems fine to me but you might know of an easier way?

[AArnott]

I don't usually redirect using the Begin methods. I just use the streams directly, and I don't assume that the streams are fully read by the time the process exits -- the readers will read till the streams hit EOF, and I've written code at times to block my process-exited processing until those EOFs are hit, rather than blocking the WaitForExit call to when Begin is called.

A couple nits though:

1. Use WaitForExitAsync instead of WaitForExit so you don't block a thread unnecessarily.
2. Use AsyncManualResetEvent instead, so you can asynchronously await there too. I think ManualResetEventSlim supports an async wait too, actually.

[mslukebo]

I don't usually redirect using the Begin methods. I just use the streams directly, and I don't assume that the streams are fully read by the time the process exits -- the readers will read till the streams hit EOF, and I've written code at times to block my process-exited processing until those EOFs are hit, rather than blocking the WaitForExit call to when Begin is called.

A couple nits though:

1. Use WaitForExitAsync instead of WaitForExit so you don't block a thread unnecessarily.
2. Use AsyncManualResetEvent instead, so you can asynchronously await there too. I think ManualResetEventSlim supports an async wait too, actually.

The only AsyncManualResetEvent I can find is in the Visual Studio namespace, which this code doesn't have access to. ManualResetEventSlim doesn't have an async wait. I can change my code to use a SemaphoreSlim which does have a WaitAsync call if that's preferred. However, I don't see the reason to add more async/await overhead with that/WaitForExitAsync. This code is already running on a background thread since it's inside a Task.Run.

I don't assume that the streams are fully read by the time the process exits -- the readers will read till the streams hit EOF, and I've written code at times to block my process-exited processing until those EOFs are hit

I agree - this code can probably be refactored as to not rely on undocumented behavior of WaitForExit. I wanted to fix the code with the minimal amount of changes though, since I was hoping to have it backported to .NET 6.

[AArnott]

The only AsyncManualResetEvent I can find is in the Visual Studio namespace, which this code doesn't have access to.

It could though, if you add a PackageReference to it. :) The dependency isn't VS-specific.

However, I don't see the reason to add more async/await overhead with that/WaitForExitAsync. This code is already running on a background thread since it's inside a Task.Run.

Thread pool threads aren't free. But as you're in a special purpose process, how you spend them is up to you. In VS, we find that blocking threadpool threads cause ui delays a lot.

[mslukebo]

The only AsyncManualResetEvent I can find is in the Visual Studio namespace, which this code doesn't have access to.

It could though, if you add a PackageReference to it. :) The dependency isn't VS-specific.

However, I don't see the reason to add more async/await overhead with that/WaitForExitAsync. This code is already running on a background thread since it's inside a Task.Run.

Thread pool threads aren't free. But as you're in a special purpose process, how you spend them is up to you. In VS, we find that blocking threadpool threads cause ui delays a lot.

Sorry, I'm not sure what you're suggesting for this PR then.

I think the options here are

1. Keep the Task.Run(Action) with synchronous method calls (what this PR currently does)
2. Move to using Task.Run(Func<Task>) and use async method calls
3. Get rid of the Task.Run entirely, switch to use an async void event handler, and use async method calls

I don't see the point in doing option 2 since we incur the cost of a thread pool thread and the overhead of async/await. I am fine changing this PR to use option 3, but like I said I was trying to minimize code changes 😃

[AArnott]

I don't think any change is required.
But FWIW, the overhead of async/await is much less than the cost incurred from blocking threadpool threads, if you hit threadpool starvation. But I suspect you won't in a non-interactive console application. And launching a process is fairly heavy anyway so I don't think you'll hit this code frequently.
And I don't like async void methods, since an exception will crash the application.

[JoeRobich]

It could though, if you add a PackageReference to it. :) The dependency isn't VS-specific.

dotnet-format has to be source buildable because it ships in the SDK. I would suspect VS.Threading likely isn't source buildable.

[AArnott]

VS.Threading likely isn't source buildable.

Certainly not by the definition that you're using.

[mslukebo]

@sharwell are there any changes you would want on this PR? @AArnott mentioned above that they do not think any changes are required. There mentioned, there are better ways to do this that would require refactoring a lot of how this works, but since there are no tests for these methods I did not want to introduce too many changes.