Implement WaitForExitAsync for System.Diagnostics.Process (#1278)
authorMatt Kotsenas <Matt.Kotsenas@gmail.com>
Thu, 13 Feb 2020 02:16:21 +0000 (21:16 -0500)
committerGitHub <noreply@github.com>
Thu, 13 Feb 2020 02:16:21 +0000 (18:16 -0800)
commitad7e1ff23b71ae246148bdfcf2711b3ed930f7ed
tree1f51c204ba8b0fca22e460b14742637698c1b79e
parentc89e27566358eff0a3b81e17e0471b88f9cfa2e5
Implement WaitForExitAsync for System.Diagnostics.Process (#1278)

* Add Process.WaitForExitAsync() and associated unit tests

Add a task-based `WaitForExitAsync()` for the `Process` class, and
duplicate existing `WaitForExit()` tests to add async versions.

In order to ensure that the `Exited` event is never lost, it is the
callers responsibility to ensure that `EnableRaisingEvents` is `true`
_prior_ to calling `WaitForExitAsync()`. A new
`InvalidOperationException` is introduced to enforce those semantics.

* Review feedback: Change WaitForExitAsync to return Task

Per review feedback, change `WaitForExitAsync` to return a `Task`
instead of `Task<bool>`. Now, to determine if the process has exited,
callers should check `HasExited` after the await, and cancellation
follows the async pattern of setting canceled on the task.

* Remove asserts on Task properties

Per code review feedback, remove the asserts that verify that the Task
returned by WaitForExitAsync is completed successfully / canceled, as
they're essentially Task tests and not relevant to WaitForExitAsync.

* Fix unit test to not create async void

Per code review feedback, fix the
MultipleProcesses_ParallelStartKillWaitAsync test to make work a
`Func<Task>` instead of an `Action` since we're await-ing it.

* Remove implicit delegate creation for ExitHandler

Per code review feedback, converting ExitHandler from a local function
to an `EventHandler` to avoid the extra delegate allocation to convert
between the types.

* Flow CancellationToken to OperationCanceledExceptions

Per code review, register the `TaskCompletionSource` with the
`CancellationToken` so that if an `OperationCanceledException` is thrown
the relevant token is available on
`OperationCanceledException.CancellationToken`.

To accomplish that the `TaskCompletionSourceWithCancellation` class
that's internal to `System.Net.Http` is moved to Common and consumed in
both places.

Unit tests are also updated to verify that the cancellation token passed
in is the same one returned from
`OperationCanceledException.CancellationToken`.

* Do not require EnableRaisingEvents to already be true

Per code review feedback, update `WaitForExitAsync` to not require the
user to call `EnableRaisingEvents` first.

Setting `EnableRaisingEvents` ourselves introduces the chance for an
exception in the case where the process has already exited, so I've
added a comment to the top of the method to detail the possible paths
through the code and added comment annotations for each case.

Lastly, I updated the unit tests to remove `EnableRaisingEvents` calls.

* Follow style guidelines in ProcessWaitingTests

Per code review feedback, update the new unit tests in
ProcessWaitingTests to follow style guidelines.

* Update tests to follow coding guidelines

Per code review feedback, update tests to follow coding guidelines,
simplify creating cancellation tokens, and verify synchronous completion
of tasks in the exited / canceled case.

* Update WaitForExitAsync to early out in canceled case

Per code review feedback, add an early out for a non-exited process when
canceled.

* Add a test for completion without a cancellation token

Per code review feedback, add a test for a process that completes
normally and doesn't use a canellation token.

* Address PR feedback in xmldocs

Per code review feedback, update the xmldocs for `WaitForExitAsync` to
specify the language keyword for "true", and add a `<returns>` element.

* Update xmldocs per code review feedback

Per code review feedback, update xmldocs to list other conditions that
can cause the function to return.

* Update function guards per code review feedback

Per code review feedback, update the method guards to span multiple
lines to adhere to style guidelines.

* Refactor test to verify async as well as sync cancellation

Per code review feedback, update
SingleProcess_TryWaitAsyncMultipleTimesBeforeCompleting to test both the
case where the token is canceled before creating the Task as well as
after the task is already created.
src/libraries/Common/src/System/Threading/Tasks/TaskCompletionSourceWithCancellation.cs [moved from src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/TaskCompletionSourceWithCancellation.cs with 73% similarity]
src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs
src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs
src/libraries/System.Diagnostics.Process/tests/ProcessTestBase.cs
src/libraries/System.Diagnostics.Process/tests/ProcessWaitingTests.cs
src/libraries/System.Net.Http/src/System.Net.Http.csproj