Question about Process redirected output and WaitForExit

[Evangelink]

Hi!

I am investigating an issue on vstest and I have nailed down the issue to the fact we receive the exit callback before we have flushed all the output/error messages.

Looking here, I have found the issue When Process.WaitForExitCore(int milliseconds) waits successfully, the redirected output streams miss some data which seems to solve our problem.

But, I am not sure which of the discussed solution is the one to rely apply (i.e. WaitForExitAsync, multi calls to WaitForExit, or read until null). The various solutions seem to work but I guess there is one solution to favor and it's not clear what/why.
I think that I simply need to replace the call to p.WaitForExit(500) by an awaited call to WaitForExitAsync with a cancellation token that times out.

Thank you for your feedback/answer.

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Hi!

I am investigating an issue on vstest and I have nailed down the issue to the fact we receive the exit callback before we have flushed all the output/error messages.

Looking here, I have found the issue When Process.WaitForExitCore(int milliseconds) waits successfully, the redirected output streams miss some data which seems to solve our problem.

But, I am not sure which of the discussed solution is the one to rely apply (i.e. WaitForExitAsync, multi calls to WaitForExit, or read until null). The various solutions seem to work but I guess there is one solution to favor and it's not clear what/why.
I think that I simply need to replace the call to p.WaitForExit(500) by an awaited call to WaitForExitAsync with a cancellation token that times out.

Thank you for your feedback/answer.

Author:	Evangelink
Assignees:	-
Labels:	question, area-System.Diagnostics.Process, untriaged
Milestone:	-

[Evangelink]

Sorry for the direct ping @tmds, but as you replied to the linked thread, you might have some feedback.

Thank you!

[tmds]

I think that I simply need to replace the call to p.WaitForExit(500) by an awaited call to WaitForExitAsync with a cancellation token that times out.

Yes, if you are doing things async that is the way to go.

await process.WaitForExitAsync(cancellationToken);

And if you're doing things sync you can:

if (process.WaitForExit(timeout))
    process.WaitForExit(); // read till EOF

(edit) This can also block when grand children are keeping the output alive.

or read until null

The read until null method is good if you know for sure the child process will exit and won't have grand children that keep the output alive after the direct child has died.

[Evangelink]

As you may have seen, we do the call in an event handler on the Exited event: https://github.com/microsoft/vstest/blob/f8d6b9363b46b3afbaf2fa07a27ceb1534433af5/src/Microsoft.TestPlatform.PlatformAbstractions/common/System/ProcessHelper.cs?rgh-link-date=2022-02-21T10%3A50%3A37Z#L83-L98

so we can go with the await I guess.

(edit) This can also block when grand children are keeping the output alive.

Regarding this comment, does it still apply to the async overload? Because that would be a problem for us to have the test stuck indefinitely because of a misconfigured grandchild.

[tmds]

Regarding this comment, does it still apply to the async overload? Because that would be a problem for us to have the test stuck indefinitely because of a misconfigured grandchild.

The async overload doesn't have the problem: the reading also gets cancelled by the token.

[Evangelink]

I will go with the async overload for net5+ as it seems to be the most convenient.

And if you're doing things sync you can:

if (process.WaitForExit(timeout))
    process.WaitForExit(); // read till EOF

(edit) This can also block when grand children are keeping the output alive.

For older frameworks, I guess what you describe above is our best solution. Are you aware of anything we could use to avoid being indefinitely stuck on the second call? Could we borrow part of the implementation of the new api? Or use an helper like https://gist.github.com/Daniel15/097c9a09bc804285da2eda8468d64474?

[tmds]

Or use an helper like https://gist.github.com/Daniel15/097c9a09bc804285da2eda8468d64474?

This is not enough since you want to give some more time for data to come in after the exited event.

Could we borrow part of the implementation of the new api?

Yes.

Are you aware of anything we could use to avoid being indefinitely stuck on the second call?

Instead of process.WaitForExit(), use a ManualResetEvent and wait with a timeout. When you read null, you can set the MRE.

[Evangelink]

@tmds If I may ask a final question, can you foresee any problem using a Task.Run for the older frameworks?

var tokenSource = new CancellationTokenSource(500);
#if NET5_0_OR_GREATER
await p?.WaitForExitAsync(tokenSource.Token);
#else
await Task.Run(() => p?.WaitForExit(), tokenSource.Token);
#endif

For the failing case I have, that seems to solve the issue and gives a relatively easy to read/understand code. But as you have more experience and insight you might think of something off with such solution.