dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.24k stars 1.35k forks source link

Process.WaitForExit() deadlocks when waiting on "dotnet.exe build /m /nr:true" #2981

Open mikeharder opened 6 years ago

mikeharder commented 6 years ago

Repro Steps

  1. Build and run https://github.com/mikeharder/nodereuse-waitforexit. A. Uses msbuild.exe and dotnet.exe from %PATH%.
  2. WaitForExit() works fine with msbuild.exe /m /nr:true.
  3. WaitForExit() deadlocks with dotnet.exe build /m /nr:true.

Root Cause

WaitForExit() has a bug/behavior where it will wait for all child processes to exit if the parent process output is being read asynchronously (https://stackoverflow.com/a/37983587/102052). The workaround is to call WaitForExit(int.MaxValue) instead, which does not wait for the child processes to exit.

Conclusion

I created this issue for MSBuild since the behavior is inconsistent between msbuild.exe and dotnet.exe build. If the only impact of this inconsistency is WaitForExit(), it's probably a low priority to change. However, it could be a symptom of other possible problems with the way dotnet.exe child processes are created.

Kuinox commented 3 years ago

Hello, I hit a bug similar to what I described in https://github.com/dotnet/runtime/issues/41752 and I think this is due to this issue.

Kuinox commented 3 years ago

The Process class creates pipes for stderr/stdout. These pipes are wrapped in a FileStream then in a StreamWriter/Reader, then in an AsyncStreamReader. Now, what the deadlock is waiting for ? WaitForExit() => WaitForExit(Timeout.Infinite) => WaitForExitCore(Timeout.Infinite) https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs#L186 WaitForExitCore, on Windows, wait on the task EOF, this Task is in fact the background read Task of the AsyncStreamReader. This background task will stop when the Pipe is closed:
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/AsyncStreamReader.cs#L96

So the issue is that we are waiting a stdout/stderr Pipe that won't close, the child process itself is gone.

akoeplinger commented 4 months ago

Reopening since #10297 had to be reverted: https://github.com/dotnet/msbuild/pull/10395

Kuinox commented 4 months ago

Like I commented in the initial PR, I think the ideal way is to fix the root bug, the issue where I reported this bug on the runtime repo https://github.com/dotnet/runtime/issues/51277 have been in needs-further-triage for 3 years.

MichalPavlik commented 4 months ago

Yes, I would like to have a fix in runtime, but the workaround you proposed was simple and it worked when I tried it. I decided it would be faster to use it now without waiting for runtime patch. Unfortunately, there are some consequences I didn't predict.

Kuinox commented 4 months ago

but the workaround you proposed was simple and it worked when I tried it

I did warn it right below the workaround that it would truncate the output.

without waiting for runtime patch

Well if nobody triage the issue I don't think the patch will be done anytime soon.