Fix deadlock in AzureCliCredential and other process based TokenCredential implementations

[likemike91]

This PR fixes an issue where large tokens (e.g. due to being member of a lot of groups in AAD) caused all TokenCredential implementations which use the ProcessRunner to fail with a Timeout due to a deadlock when reading the StandardOut stream.

[ghost]

Thank you for your contribution likemike91! We will review the pull request and get back to you soon.

[ghost]

All CLA requirements met.

[likemike91]

This fixes #14691 and #14207

[jsquire]

Hi @likemike91. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. Tagging and routing to the team members best able to provide feedback.

[jsquire]

@schaabs: Would you please be so kind as to offer your feedback?

[schaabs]

@likemike91 Thanks for your contribution helping us address this issue which is impacting lots of users! Apologies on the delay getting you feedback. I have a couple of high level comments on this approach. First, the current implementation of ProcessRunner is non-blocking, but using AutoResetEvent for synchronization introduces blocking calls. Did you consider using TaskCompletionSource rather than AutoResetEvent to signal when the output is complete so that it can be awaited rather than blocking? Secondly, this change complicates the IProcess abstraction quite a bit, and requires implementing events to mock. Since, in the end we, read the process output and error to a string, perhaps we could simplify the abstraction by just exposing the stdout and stderr content as string properties? Again, thank you for the contribution, and please let me know if you have any questions or thoughts on the approaches I outlined.

[likemike91]

@schaabs Thank you for your feedback on my implementation. I now switched over the implementation to TaskCompletionSource to be non-blocking respectively awaitable (90bf469). With the last commit (e76cce2) I tried to overcome some race conditions where the process.Exited event is received before the last process.OutputDataReceived or process.ErrorDataReceived event is received.
Regarding your second suggestion to move the event handling stuff into the ProcessWrapper itself I can't think of a simple solution without totally rewriting the whole ProcessRunner/IProcess stuff since I also have to handle those race conditions mentioned before as well. Maybe I'm thinking too complicated right now but if so maybe you can point me into the right direction? 🙈

PS: I added two cleanup commits 🙈 (0bab3ed, a05f4b8)

[AlexanderSher]

@likemike91, I really am sorry that I've left these issues unresolved before going on a long leave. Thank you very much for your contribution.
@schaabs Sorry, I've completely forgotten about buffer overflow when I wrote ProcessRunner. Unfortunately, the only way we can read asynchronously from Process is by using OutputDataReceived and ErrorDataReceived events. I've created a PR that addresses this issue and also handles the case when Process.Start returns false.

[likemike91]

@AlexanderSher Okay thank you for your reply. Then I'll assume I can Close/Remove my PR?

[likemike91]

Closed due to #16499