Skip to content

Commit fbce074

Browse files
EvangelinkCopilot
andcommitted
Adopt CancellationToken + IDisposable on ITestHostHandle; doc nits
- ITestHostHandle now extends IDisposable and WaitForExitAsync takes a CancellationToken (both supported by the runtime and the netstandard2.0 polyfill). The platform disposes the handle after exit and passes its cancellation token while waiting, reconciling the real exit code afterwards. - Document ExitCode-before-HasExited as undefined; note Quote/PasteArguments are placeholders for proper argument quoting; fix the container example so Terminate() tears down the container (docker stop) rather than only killing the local client. - Record the design evolution in Alternatives. Implemented in PR #9454. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 278c3e4 commit fbce074

1 file changed

Lines changed: 38 additions & 8 deletions

File tree

docs/RFCs/017-TestHost-Launcher.md

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -147,23 +147,33 @@ monitoring contract):
147147

148148
```csharp
149149
[Experimental("TPEXP", UrlFormat = "https://aka.ms/testingplatform/diagnostics#{0}")]
150-
public interface ITestHostHandle
150+
public interface ITestHostHandle : IDisposable
151151
{
152152
/// <summary>
153153
/// Free-form diagnostic identifier (a PID, container id, remote host:pid, …) or null. The
154154
/// platform never relies on it for control flow.
155155
/// </summary>
156156
string? Identifier { get; }
157157

158+
/// <summary>Only valid once <see cref="HasExited"/> is true (reading it earlier is undefined).</summary>
158159
int ExitCode { get; }
159160
bool HasExited { get; }
160-
Task WaitForExitAsync();
161+
162+
/// <summary>Waits for exit, or for the token to be canceled. May be awaited more than once.</summary>
163+
Task WaitForExitAsync(CancellationToken cancellationToken);
161164

162165
/// <summary>Best-effort teardown (e.g. when hang dump aborts the run).</summary>
163166
void Terminate();
164167
}
165168
```
166169

170+
`ITestHostHandle` extends `IDisposable`: the platform owns the handle for the whole lifetime of the
171+
test host and disposes it once the host has exited, so implementations release any OS resources they
172+
hold (process objects, sockets, container clients, …) in `Dispose`. `WaitForExitAsync` takes a
173+
`CancellationToken` so the platform can stop waiting on cancellation; the controller host still
174+
reconciles the real OS exit code afterwards (on cancellation it terminates the host and waits for it
175+
to fully exit).
176+
167177
Registration mirrors the existing methods on `ITestHostControllersManager`:
168178

169179
```csharp
@@ -212,12 +222,23 @@ public interface ITestHostControllersManager
212222
environment variables, so a packaged-app launcher must transfer them another way.
213223
- The returned handle must report exit reliably (`WaitForExitAsync`, `ExitCode`, `HasExited`) and
214224
support `Terminate()` (hang dump terminates the host through it). `WaitForExitAsync` may be awaited
215-
more than once.
225+
more than once, and must honor its `CancellationToken`. `ExitCode` is only required to be valid
226+
once `HasExited` is `true` (or after `WaitForExitAsync` completes); reading it on a still-running
227+
handle is undefined and implementations are not required to throw.
228+
- The handle is `IDisposable`; the platform disposes it once the host has exited, so the launcher
229+
should release any OS resources it holds (process object, sockets, container client, …) in
230+
`Dispose`.
216231
- `Identifier` is an optional free-form diagnostic string (PID, container id, remote `host:pid`, …)
217232
and may be `null`. The platform never relies on it for control flow.
218233
- If the launcher cannot start the host it should throw; the platform surfaces it as a
219234
platform-setup failure.
220235

236+
> The `Quote` and `PasteArguments` helpers used in the examples below are placeholders for whatever
237+
> argument/shell quoting the target mechanism needs (e.g. `PasteArguments` from dotnet/runtime for
238+
> Windows command lines, POSIX single-quoting for a shell). Implement them carefully to avoid
239+
> argument-injection bugs; the reference `Microsoft.Testing.Extensions.PackagedApp` extension (see the
240+
> implementation PR) shows a concrete approach.
241+
221242
## Examples
222243

223244
All examples assume the extension is registered on the builder, e.g. from a `…Extensions` helper:
@@ -307,8 +328,11 @@ requires that the host ends up with `context.EnvironmentVariables`.
307328

308329
### 4. Container
309330

310-
Run the test host inside a container and bridge the pipe. The returned handle tracks the
311-
`docker run` client process; `Terminate()` tears down the container.
331+
Run the test host inside a container and bridge the pipe. `docker run --rm` is used so the container
332+
is removed when it stops. The returned handle tracks the `docker run` client process; note that
333+
killing the client alone does not reliably stop the container, so a real implementation should make
334+
`Terminate()` run `docker stop`/`docker rm` (or run with `--init` and rely on `--rm`) rather than
335+
just killing the local client.
312336

313337
```csharp
314338
public Task<ITestHostHandle> LaunchTestHostAsync(
@@ -317,14 +341,19 @@ public Task<ITestHostHandle> LaunchTestHostAsync(
317341
var args = new List<string> { "run", "--rm", "--init" };
318342
foreach (var kvp in context.EnvironmentVariables.Where(kv => kv.Value is not null)) { args.Add("-e"); args.Add($"{kvp.Key}={kvp.Value}"); } // skip unset (null) vars
319343
// Map the controller pipe into the container (Windows named pipe / Unix domain socket mount).
344+
args.Add("--name");
345+
string containerName = $"mtp-{Guid.NewGuid():N}";
346+
args.Add(containerName);
320347
args.Add("test-image:latest");
321348
args.Add(context.FileName);
322349
args.AddRange(context.Arguments);
323350

324351
var psi = new ProcessStartInfo("docker") { UseShellExecute = false };
325352
foreach (string a in args) psi.ArgumentList.Add(a);
326353
Process p = Process.Start(psi)!;
327-
return Task.FromResult<ITestHostHandle>(new ProcessHandleAdapter(p));
354+
// Wrap so Terminate() runs `docker stop <containerName>` (which tears down the container), not
355+
// just Kill() on the local docker client.
356+
return Task.FromResult<ITestHostHandle>(new DockerRunHandle(p, containerName));
328357
}
329358
```
330359

@@ -376,8 +405,9 @@ An earlier draft of this RFC named the hook `ITestHostProcessLauncher` and retur
376405
is a local OS process," which is false for container and remote launches and awkward for
377406
AUMID-activated apps. The current design renames the types to drop "Process", replaces the `int`
378407
process id with an optional free-form `string Identifier` (diagnostic only), drops the redundant
379-
`Exited` event in favour of `WaitForExitAsync`, and names the teardown `Terminate()` instead of
380-
`Kill()`.
408+
`Exited` event in favour of `WaitForExitAsync`, gives `WaitForExitAsync` a `CancellationToken` and
409+
makes the handle `IDisposable` (so the platform can honor cancellation and deterministically release
410+
handle resources), and names the teardown `Terminate()` instead of `Kill()`.
381411

382412
### Do nothing (keep `Process.Start`)
383413

0 commit comments

Comments
 (0)