Skip to content

Commit ad7e1ff

Browse files
MattKotsenasMattKotsenas
authored
Implement WaitForExitAsync for System.Diagnostics.Process (dotnet#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.
1 parent c89e275 commit ad7e1ff

7 files changed

Lines changed: 491 additions & 10 deletions

File tree

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/TaskCompletionSourceWithCancellation.cs renamed to src/libraries/Common/src/System/Threading/Tasks/TaskCompletionSourceWithCancellation.cs

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33
// See the LICENSE file in the project root for more information.
44

5-
using System.Threading;
6-
using System.Threading.Tasks;
7-
8-
namespace System.Net.Http
5+
namespace System.Threading.Tasks
96
{
7+
/// <summary>
8+
/// A <see cref="TaskCompletionSource{TResult}"/> that supports cancellation registration so that any
9+
/// <seealso cref="OperationCanceledException"/>s contain the relevant <see cref="CancellationToken"/>,
10+
/// while also avoiding unnecessary allocations for closure captures.
11+
/// </summary>
1012
internal class TaskCompletionSourceWithCancellation<T> : TaskCompletionSource<T>
1113
{
1214
private CancellationToken _cancellationToken;

‎src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ public void Refresh() { }
123123
public override string ToString() { throw null; }
124124
public void WaitForExit() { }
125125
public bool WaitForExit(int milliseconds) { throw null; }
126+
public System.Threading.Tasks.Task WaitForExitAsync(System.Threading.CancellationToken cancellationToken = default) { throw null; }
126127
public bool WaitForInputIdle() { throw null; }
127128
public bool WaitForInputIdle(int milliseconds) { throw null; }
128129
}

‎src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,9 @@
4242
<Compile Include="$(CommonPath)Interop\Windows\Interop.Errors.cs">
4343
<Link>Common\Interop\Windows\Interop.Errors.cs</Link>
4444
</Compile>
45+
<Compile Include="$(CommonPath)System\Threading\Tasks\TaskCompletionSourceWithCancellation.cs">
46+
<Link>Common\System\Threading\Tasks\TaskCompletionSourceWithCancellation.cs</Link>
47+
</Compile>
4548
</ItemGroup>
4649
<ItemGroup Condition=" '$(TargetsWindows)' == 'true'">
4750
<Compile Include="$(CommonPath)Interop\Windows\Kernel32\Interop.EnumProcessModules.cs">

‎src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.cs‎

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
using System.Runtime.Serialization;
1111
using System.Text;
1212
using System.Threading;
13+
using System.Threading.Tasks;
1314

1415
namespace System.Diagnostics
1516
{
@@ -1346,6 +1347,107 @@ public bool WaitForExit(int milliseconds)
13461347
return exited;
13471348
}
13481349

1350+
/// <summary>
1351+
/// Instructs the Process component to wait for the associated process to exit, or
1352+
/// for the <paramref name="cancellationToken"/> to be canceled.
1353+
/// </summary>
1354+
/// <remarks>
1355+
/// Calling this method will set <see cref="EnableRaisingEvents"/> to <see langword="true" />.
1356+
/// </remarks>
1357+
/// <returns>
1358+
/// A task that will complete when the process has exited, cancellation has been requested,
1359+
/// or an error occurs.
1360+
/// </returns>
1361+
public async Task WaitForExitAsync(CancellationToken cancellationToken = default)
1362+
{
1363+
// Because the process has already started by the time this method is called,
1364+
// we're in a race against the process to set up our exit handlers before the process
1365+
// exits. As a result, there are several different flows that must be handled:
1366+
//
1367+
// CASE 1: WE ENABLE EVENTS
1368+
// This is the "happy path". In this case we enable events.
1369+
//
1370+
// CASE 1.1: PROCESS EXITS OR IS CANCELED AFTER REGISTERING HANDLER
1371+
// This case continues the "happy path". The process exits or waiting is canceled after
1372+
// registering the handler and no special cases are needed.
1373+
//
1374+
// CASE 1.2: PROCESS EXITS BEFORE REGISTERING HANDLER
1375+
// It's possible that the process can exit after we enable events but before we reigster
1376+
// the handler. In that case we must check for exit after registering the handler.
1377+
//
1378+
//
1379+
// CASE 2: PROCESS EXITS BEFORE ENABLING EVENTS
1380+
// The process may exit before we attempt to enable events. In that case EnableRaisingEvents
1381+
// will throw an exception like this:
1382+
// System.InvalidOperationException : Cannot process request because the process (42) has exited.
1383+
// In this case we catch the InvalidOperationException. If the process has exited, our work
1384+
// is done and we return. If for any reason (now or in the future) enabling events fails
1385+
// and the process has not exited, bubble the exception up to the user.
1386+
//
1387+
//
1388+
// CASE 3: USER ALREADY ENABLED EVENTS
1389+
// In this case the user has already enabled raising events. Re-enabling events is a no-op
1390+
// as the value hasn't changed. However, no-op also means that if the process has already
1391+
// exited, EnableRaisingEvents won't throw an exception.
1392+
//
1393+
// CASE 3.1: PROCESS EXITS OR IS CANCELED AFTER REGISTERING HANDLER
1394+
// (See CASE 1.1)
1395+
//
1396+
// CASE 3.2: PROCESS EXITS BEFORE REGISTERING HANDLER
1397+
// (See CASE 1.2)
1398+
1399+
if (!Associated)
1400+
{
1401+
throw new InvalidOperationException(SR.NoAssociatedProcess);
1402+
}
1403+
1404+
if (!HasExited)
1405+
{
1406+
// Early out for cancellation before doing more expensive work
1407+
cancellationToken.ThrowIfCancellationRequested();
1408+
}
1409+
1410+
try
1411+
{
1412+
// CASE 1: We enable events
1413+
// CASE 2: Process exits before enabling events (and throws an exception)
1414+
// CASE 3: User already enabled events (no-op)
1415+
EnableRaisingEvents = true;
1416+
}
1417+
catch (InvalidOperationException)
1418+
{
1419+
// CASE 2: If the process has exited, our work is done, otherwise bubble the
1420+
// exception up to the user
1421+
if (HasExited)
1422+
{
1423+
return;
1424+
}
1425+
1426+
throw;
1427+
}
1428+
1429+
var tcs = new TaskCompletionSourceWithCancellation<object>();
1430+
1431+
EventHandler handler = (s, e) => tcs.TrySetResult(null);
1432+
Exited += handler;
1433+
1434+
try
1435+
{
1436+
if (HasExited)
1437+
{
1438+
// CASE 1.2 & CASE 3.2: Handle race where the process exits before registering the handler
1439+
return;
1440+
}
1441+
1442+
// CASE 1.1 & CASE 3.1: Process exits or is canceled here
1443+
await tcs.WaitWithCancellationAsync(cancellationToken).ConfigureAwait(false);
1444+
}
1445+
finally
1446+
{
1447+
Exited -= handler;
1448+
}
1449+
}
1450+
13491451
/// <devdoc>
13501452
/// <para>
13511453
/// Instructs the <see cref='System.Diagnostics.Process'/> component to start

‎src/libraries/System.Diagnostics.Process/tests/ProcessTestBase.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,18 @@ protected Process CreateProcess(Func<int> method = null)
6060
return p;
6161
}
6262

63+
protected Process CreateProcess(Func<Task<int>> method)
64+
{
65+
Process p = null;
66+
using (RemoteInvokeHandle handle = RemoteExecutor.Invoke(method, new RemoteInvokeOptions { Start = false }))
67+
{
68+
p = handle.Process;
69+
handle.Process = null;
70+
}
71+
AddProcessForDispose(p);
72+
return p;
73+
}
74+
6375
protected Process CreateProcess(Func<string, int> method, string arg, bool autoDispose = true)
6476
{
6577
Process p = null;

0 commit comments

Comments
 (0)