Fix deadlock in ProcessUtil

[karolz-ms]

Fixes #1420

Microsoft Reviewers: Open in CodeFlow

[JamesNK]

Unit test?

[karolz-ms]

Unit test?

Honestly I am not sure what kind of unit test would make sense for this change. Any suggestions?

[JamesNK]

Is it possible to re-create the deadlock in a test? If so, have a test of that scenario that now passes.

[BrennanConroy]

Is it possible to re-create the deadlock in a test?

Not without private reflection into the internals of Process and adding hooks to our callbacks just for testing.

[JamesNK]

So you're saying there's a chance...

ok, nvm

[BrennanConroy]

Yes 😆

[karolz-ms]

Since here we only care about startup activities to be completed before anything else happens, I can think of a better fix, using manual reset event. Stay tuned...

[JamesNK]

Is the problem in #1420 that HasExited can cause the process to check whether the process has exited, realize it has, and then run the Exited event inline?

What is the purpose of checking HasExited in the OutputDataReceived/ErrorDataReceived events? Could we change it so the Exited event sets a private bool _hasExited to true, and then OutputDataReceived/ErrorDataReceived checks _hasExited to see whether an exit has been triggered? The field wouldn't be quite as up to date as HasExited, but does that matter?

[karolz-ms]

@JamesNK I think my second attempt is both more robust and easier to understand from the intention perspective. LMK what you think!

[JamesNK]

@JamesNK I think my second attempt is both more robust and easier to understand from the intention perspective. LMK what you think!

I like it more. It needs some comments explaining why it works the way it does.

[karolz-ms]

Thanks @JamesNK!