Closed mslukebo closed 11 months ago
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?
@AArnott this seems fine to me but you might know of an easier way?
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:
WaitForExitAsync
instead of WaitForExit
so you don't block a thread unnecessarily.AsyncManualResetEvent
instead, so you can asynchronously await there too. I think ManualResetEventSlim
supports an async wait too, actually.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:
- Use
WaitForExitAsync
instead ofWaitForExit
so you don't block a thread unnecessarily.- Use
AsyncManualResetEvent
instead, so you can asynchronously await there too. I thinkManualResetEventSlim
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.
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.
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
Task.Run(Action)
with synchronous method calls (what this PR currently does)Task.Run(Func<Task>)
and use async method callsTask.Run
entirely, switch to use an async void
event handler, and use async method callsI 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 😃
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.
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.
VS.Threading likely isn't source buildable.
Certainly not by the definition that you're using.
@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.
If
Process.WaitForExit
is called beforeBeginOutputReadLine
and/orBeginErrorReadLine
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 betweenProcess.Start
andBeginOutputReadLine
/BeginErrorReadLine
, it's possible to not receive any output before the process exits and theTaskCompletionSource
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.