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

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

Open
mikeharder opened this issue Feb 9, 2018 · 6 comments · Fixed by #10297
Open

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

mikeharder opened this issue Feb 9, 2018 · 6 comments · Fixed by #10297
Assignees
Labels

Comments

@mikeharder
Copy link

mikeharder commented Feb 9, 2018

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
Copy link

Kuinox commented Apr 12, 2021

Hello, I hit a bug similar to what I described in dotnet/runtime#41752 and I think this is due to this issue.

@Kuinox
Copy link

Kuinox commented Apr 13, 2021

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
Copy link
Member

Reopening since #10297 had to be reverted: #10395

@akoeplinger akoeplinger reopened this Jul 17, 2024
@Kuinox
Copy link

Kuinox commented Jul 17, 2024

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 dotnet/runtime#51277 have been in needs-further-triage for 3 years.

@MichalPavlik
Copy link
Member

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
Copy link

Kuinox commented Jul 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants