Change Processes to use .WaitForExitAsync()

- Remove custom exit handler.
- Move exit handling into `Tgstation.Server.Host.System.Process`.
- Adjust `IProcessExecutor.LaunchNew` signature.
- `IProcessBase.Lifetime` result now nullable to represent a detach.
- Await `Lifetime` in dispose.

src/Tgstation.Server.Host.Watchdog/Watchdog.cs

@@ -143,17 +143,10 @@ namespace Tgstation.Server.Host.Watchdog
 
 							process.StartInfo.UseShellExecute = false; // runs in the same console
 
-							var tcs = new TaskCompletionSource<object>();
-							process.Exited += (a, b) =>
-							{
-								tcs.TrySetResult(null);
-							};
-							process.EnableRaisingEvents = true;
-
 							var killedHostProcess = false;
 							try
 							{
-								var processTask = tcs.Task;
+								Task processTask = null;
 								(int, Task) StartProcess(string additionalArg)
 								{
 									if (additionalArg != null)
@@ -162,11 +155,10 @@ namespace Tgstation.Server.Host.Watchdog
 									logger.LogInformation("Launching host with arguments: {arguments}", process.StartInfo.Arguments);
 
 									process.Start();
-									return (process.Id, processTask);
+									return (process.Id, processTask = process.WaitForExitAsync(cancellationToken));
 								}
 
 								using (var processCts = new CancellationTokenSource())
-								using (processCts.Token.Register(() => tcs.TrySetResult(null)))
 								using (cancellationToken.Register(() =>
 								{
 									if (!Directory.Exists(updateDirectory))

src/Tgstation.Server.Host/Components/Byond/WindowsByondInstaller.cs

@@ -206,7 +206,7 @@ namespace Tgstation.Server.Host.Components.Byond
 			try
 			{
 				// noShellExecute because we aren't doing runas shennanigans
-				await using var directXInstaller = await processExecutor.LaunchProcess(
+				await using var directXInstaller = processExecutor.LaunchProcess(
 					IOManager.ConcatPath(rbdx, "DXSETUP.exe"),
 					rbdx,
 					"/silent",
@@ -214,7 +214,7 @@ namespace Tgstation.Server.Host.Components.Byond
 
 				int exitCode;
 				using (cancellationToken.Register(() => directXInstaller.Terminate()))
-					exitCode = await directXInstaller.Lifetime;
+					exitCode = (await directXInstaller.Lifetime).Value;
 				cancellationToken.ThrowIfCancellationRequested();
 
 				if (exitCode != 0)
@@ -251,7 +251,7 @@ namespace Tgstation.Server.Host.Components.Byond
 				// 1. It'd make IByondInstaller need to be transient per-instance and WindowsByondInstaller relys on being a singleton for its DX installer call
 				// 2. The instance could be renamed, so it'd have to be an unfriendly ID anyway.
 				var arguments = $"advfirewall firewall add rule name=\"TGS DreamDaemon {version}\" program=\"{dreamDaemonPath}\" protocol=tcp dir=in enable=yes action=allow";
-				await using var netshProcess = await processExecutor.LaunchProcess(
+				await using var netshProcess = processExecutor.LaunchProcess(
 					"netsh.exe",
 					IOManager.ResolvePath(),
 					arguments,
@@ -260,7 +260,7 @@ namespace Tgstation.Server.Host.Components.Byond
 
 				int exitCode;
 				using (cancellationToken.Register(() => netshProcess.Terminate()))
-					exitCode = await netshProcess.Lifetime;
+					exitCode = (await netshProcess.Lifetime).Value;
 				cancellationToken.ThrowIfCancellationRequested();
 
 				Logger.LogDebug(

src/Tgstation.Server.Host/Components/Deployment/DreamMaker.cs

@@ -18,7 +18,6 @@ using Tgstation.Server.Host.Components.Repository;
 using Tgstation.Server.Host.Components.Session;
 using Tgstation.Server.Host.Configuration;
 using Tgstation.Server.Host.Database;
-using Tgstation.Server.Host.Extensions;
 using Tgstation.Server.Host.IO;
 using Tgstation.Server.Host.Jobs;
 using Tgstation.Server.Host.Models;
@@ -860,7 +859,7 @@ namespace Tgstation.Server.Host.Components.Deployment
 		/// <returns>A <see cref="Task"/> representing the running operation.</returns>
 		async Task<int> RunDreamMaker(string dreamMakerPath, Models.CompileJob job, CancellationToken cancellationToken)
 		{
-			await using var dm = await processExecutor.LaunchProcess(
+			await using var dm = processExecutor.LaunchProcess(
 				dreamMakerPath,
 				ioManager.ResolvePath(
 					job.DirectoryName.ToString()),
@@ -873,7 +872,7 @@ namespace Tgstation.Server.Host.Components.Deployment
 
 			int exitCode;
 			using (cancellationToken.Register(() => dm.Terminate()))
-				exitCode = await dm.Lifetime;
+				exitCode = (await dm.Lifetime).Value;
 			cancellationToken.ThrowIfCancellationRequested();
 
 			logger.LogDebug("DreamMaker exit code: {exitCode}", exitCode);

src/Tgstation.Server.Host/Components/Session/SessionController.cs

@@ -67,7 +67,7 @@ namespace Tgstation.Server.Host.Components.Session
 		public Task<LaunchResult> LaunchResult { get; }
 
 		/// <inheritdoc />
-		public Task<int> Lifetime { get; }
+		public Task<int?> Lifetime { get; }
 
 		/// <inheritdoc />
 		public Task OnStartup => startupTcs.Task;
@@ -296,7 +296,7 @@ namespace Tgstation.Server.Host.Components.Session
 						? "no"
 						: $"incompatible ({reattachInformation.Dmb.CompileJob.DMApiVersion})");
 
-			async Task<int> WrapLifetime()
+			async Task<int?> WrapLifetime()
 			{
 				var exitCode = await process.Lifetime;
 				await postLifetimeCallback();

src/Tgstation.Server.Host/Components/Session/SessionControllerFactory.cs

@@ -523,7 +523,7 @@ namespace Tgstation.Server.Host.Components.Session
 					: String.Empty,
 				parameters);
 
-			var process = await processExecutor.LaunchProcess(
+			var process = processExecutor.LaunchProcess(
 				byondLock.DreamDaemonPath,
 				dmbProvider.Directory,
 				arguments,

src/Tgstation.Server.Host/Components/StaticFiles/Configuration.cs

@@ -629,7 +629,7 @@ namespace Tgstation.Server.Host.Components.StaticFiles
 				foreach (var scriptFile in scriptFiles)
 				{
 					logger.LogTrace("Running event script {scriptFile}...", scriptFile);
-					await using (var script = await processExecutor.LaunchProcess(
+					await using (var script = processExecutor.LaunchProcess(
 						ioManager.ConcatPath(resolvedScriptsDir, scriptFile),
 						resolvedScriptsDir,
 						String.Join(

src/Tgstation.Server.Host/System/IProcessBase.cs

@@ -9,9 +9,9 @@ namespace Tgstation.Server.Host.System
 	interface IProcessBase
 	{
 		/// <summary>
-		/// The <see cref="Task{TResult}"/> resulting in the exit code of the process.
+		/// The <see cref="Task{TResult}"/> resulting in the exit code of the process or <see langword="null"/> if the process was detached.
 		/// </summary>
-		Task<int> Lifetime { get; }
+		Task<int?> Lifetime { get; }
 
 		/// <summary>
 		/// Set's the owned <see cref="global::System.Diagnostics.Process.PriorityClass"/> to a non-normal value.

src/Tgstation.Server.Host/System/IProcessExecutor.cs

@@ -1,6 +1,4 @@
-using System.Threading.Tasks;
-
-namespace Tgstation.Server.Host.System
+namespace Tgstation.Server.Host.System
 {
 	/// <summary>
 	/// For launching <see cref="IProcess"/>'.
@@ -16,8 +14,8 @@ namespace Tgstation.Server.Host.System
 		/// <param name="fileRedirect">File to write process output and error streams to. Requires <paramref name="readStandardHandles"/> to be <see langword="true"/>.</param>
 		/// <param name="readStandardHandles">If the process output and error streams should be read.</param>
 		/// <param name="noShellExecute">If shell execute should not be used. Must be set if <paramref name="readStandardHandles"/> is set.</param>
-		/// <returns>A <see cref="Task"/> resulting in the new <see cref="IProcess"/>.</returns>
-		Task<IProcess> LaunchProcess(
+		/// <returns>The new <see cref="IProcess"/>.</returns>
+		IProcess LaunchProcess(
 			string fileName,
 			string workingDirectory,
 			string arguments = null,

src/Tgstation.Server.Host/System/PosixProcessFeatures.cs

@@ -88,7 +88,7 @@ namespace Tgstation.Server.Host.System
 
 			string output;
 			int exitCode;
-			await using (var gcoreProc = await lazyLoadedProcessExecutor.Value.LaunchProcess(
+			await using (var gcoreProc = lazyLoadedProcessExecutor.Value.LaunchProcess(
 				GCorePath,
 				Environment.CurrentDirectory,
 				$"-o {outputFile} {process.Id}",
@@ -96,7 +96,7 @@ namespace Tgstation.Server.Host.System
 				noShellExecute: true))
 			{
 				using (cancellationToken.Register(() => gcoreProc.Terminate()))
-					exitCode = await gcoreProc.Lifetime;
+					exitCode = (await gcoreProc.Lifetime).Value;
 
 				output = await gcoreProc.GetCombinedOutput(cancellationToken);
 				logger.LogDebug("gcore output:{0}{1}", Environment.NewLine, output);

src/Tgstation.Server.Host/System/Process.cs

@@ -20,7 +20,7 @@ namespace Tgstation.Server.Host.System
 		public Task Startup { get; }
 
 		/// <inheritdoc />
-		public Task<int> Lifetime { get; }
+		public Task<int?> Lifetime { get; }
 
 		/// <summary>
 		/// The <see cref="IProcessFeatures"/> for the <see cref="Process"/>.
@@ -38,9 +38,9 @@ namespace Tgstation.Server.Host.System
 		readonly global::System.Diagnostics.Process handle;
 
 		/// <summary>
-		/// The <see cref="CancellationTokenSource"/> used to shutdown the <see cref="readTask"/>.
+		/// The <see cref="CancellationTokenSource"/> used to shutdown the <see cref="readTask"/> and <see cref="Lifetime"/>.
 		/// </summary>
-		readonly CancellationTokenSource readerCts;
+		readonly CancellationTokenSource cancellationTokenSource;
 
 		/// <summary>
 		/// The <see cref="global::System.Diagnostics.Process.SafeHandle"/>.
@@ -58,8 +58,7 @@ namespace Tgstation.Server.Host.System
 		/// </summary>
 		/// <param name="processFeatures">The value of <see cref="processFeatures"/>.</param>
 		/// <param name="handle">The value of <see cref="handle"/>.</param>
-		/// <param name="readerCts">The value of <see cref="readerCts"/>.</param>
-		/// <param name="lifetime">The value of <see cref="Lifetime"/>.</param>
+		/// <param name="readerCts">The override value of <see cref="cancellationTokenSource"/>.</param>
 		/// <param name="readTask">The value of <see cref="readTask"/>.</param>
 		/// <param name="logger">The value of <see cref="logger"/>.</param>
 		/// <param name="preExisting">If <paramref name="handle"/> was NOT just created.</param>
@@ -67,7 +66,6 @@ namespace Tgstation.Server.Host.System
 			IProcessFeatures processFeatures,
 			global::System.Diagnostics.Process handle,
 			CancellationTokenSource readerCts,
-			Task<int> lifetime,
 			Task<string> readTask,
 			ILogger<Process> logger,
 			bool preExisting)
@@ -78,7 +76,7 @@ namespace Tgstation.Server.Host.System
 			safeHandle = handle.SafeHandle;
 			Id = handle.Id;
 
-			this.readerCts = readerCts;
+			cancellationTokenSource = readerCts ?? new CancellationTokenSource();
 
 			this.processFeatures = processFeatures ?? throw new ArgumentNullException(nameof(processFeatures));
 
@@ -86,7 +84,7 @@ namespace Tgstation.Server.Host.System
 
 			this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
 
-			Lifetime = WrapLifetimeTask(lifetime ?? throw new ArgumentNullException(nameof(lifetime)));
+			Lifetime = WrapLifetimeTask();
 
 			if (preExisting)
 			{
@@ -117,11 +115,13 @@ namespace Tgstation.Server.Host.System
 		public async ValueTask DisposeAsync()
 		{
 			logger.LogTrace("Disposing PID {pid}...", Id);
-			readerCts?.Cancel();
-			readerCts?.Dispose();
+			cancellationTokenSource.Cancel();
+			cancellationTokenSource.Dispose();
 			if (readTask != null)
 				await readTask;
 
+			await Lifetime;
+
 			safeHandle.Dispose();
 			handle.Dispose();
 		}
@@ -221,13 +221,21 @@ namespace Tgstation.Server.Host.System
 		/// <summary>
 		/// Attaches a log message to the process' exit event.
 		/// </summary>
-		/// <param name="lifetimeTask">The original lifetime <see cref="Task{TResult}"/>.</param>
-		/// <returns>A <see cref="Task{TResult}"/> functionally identical to <paramref name="lifetimeTask"/>.</returns>
-		async Task<int> WrapLifetimeTask(Task<int> lifetimeTask)
+		/// <returns>A <see cref="Task{TResult}"/> resulting in the <see cref="global::System.Diagnostics.Process.ExitCode"/> or <see langword="null"/> if the process was detached.</returns>
+		async Task<int?> WrapLifetimeTask()
 		{
-			var exitCode = await lifetimeTask;
-			logger.LogTrace("PID {pid} exited with code {exitCode}", Id, exitCode);
-			return exitCode;
+			try
+			{
+				await handle.WaitForExitAsync(cancellationTokenSource.Token);
+				var exitCode = handle.ExitCode;
+				logger.LogTrace("PID {pid} exited with code {exitCode}", Id, exitCode);
+				return exitCode;
+			}
+			catch (OperationCanceledException ex)
+			{
+				logger.LogTrace(ex, "Process lifetime task cancelled!");
+				return null;
+			}
 		}
 	}
 }

src/Tgstation.Server.Host/System/ProcessExecutor.cs

@@ -6,7 +6,6 @@ using System.Threading.Tasks;
 
 using Microsoft.Extensions.Logging;
 
-using Tgstation.Server.Host.Extensions;
 using Tgstation.Server.Host.IO;
 
 namespace Tgstation.Server.Host.System
@@ -80,7 +79,7 @@ namespace Tgstation.Server.Host.System
 		}
 
 		/// <inheritdoc />
-		public async Task<IProcess> LaunchProcess(
+		public IProcess LaunchProcess(
 			string fileName,
 			string workingDirectory,
 			string arguments,
@@ -111,41 +110,47 @@ namespace Tgstation.Server.Host.System
 				handle.StartInfo.UseShellExecute = !noShellExecute;
 
 				var processStartTcs = new TaskCompletionSource();
-				var lifetimeTaskTask = AttachExitHandlerBeforeLaunch(handle, processStartTcs.Task);
 
 				Task<string> readTask = null;
 				CancellationTokenSource disposeCts = null;
-				if (readStandardHandles)
-				{
-					handle.StartInfo.RedirectStandardOutput = true;
-					handle.StartInfo.RedirectStandardError = true;
-
-					disposeCts = new CancellationTokenSource();
-					readTask = ConsumeReaders(handle, processStartTcs.Task, fileRedirect, disposeCts.Token);
-				}
-
 				try
 				{
-					handle.Start();
+					if (readStandardHandles)
+					{
+						handle.StartInfo.RedirectStandardOutput = true;
+						handle.StartInfo.RedirectStandardError = true;
 
-					processStartTcs.SetResult();
+						disposeCts = new CancellationTokenSource();
+						readTask = ConsumeReaders(handle, processStartTcs.Task, fileRedirect, disposeCts.Token);
+					}
+
+					try
+					{
+						handle.Start();
+
+						processStartTcs.SetResult();
+					}
+					catch (Exception ex)
+					{
+						processStartTcs.SetException(ex);
+						throw;
+					}
+
+					var process = new Process(
+						processFeatures,
+						handle,
+						disposeCts,
+						readTask,
+						loggerFactory.CreateLogger<Process>(),
+						false);
+
+					return process;
 				}
-				catch (Exception ex)
+				catch
 				{
-					processStartTcs.SetException(ex);
+					disposeCts?.Dispose();
 					throw;
 				}
-
-				var process = new Process(
-					processFeatures,
-					handle,
-					disposeCts,
-					await lifetimeTaskTask, // won't block
-					readTask,
-					loggerFactory.CreateLogger<Process>(),
-					false);
-
-				return process;
 			}
 			catch
 			{
@@ -175,66 +180,6 @@ namespace Tgstation.Server.Host.System
 			return CreateFromExistingHandle(handle);
 		}
 
-		/// <summary>
-		/// Wrapper for <see cref="AttachExitHandler(global::System.Diagnostics.Process, Func{int})"/> to safely provide the process ID.
-		/// </summary>
-		/// <param name="handle">The <see cref="global::System.Diagnostics.Process"/> to attach an exit handler to.</param>
-		/// <param name="startupTask">A <see cref="Task"/> that completes once the process represented by <paramref name="handle"/> launches.</param>
-		/// <returns>The result of the call to <see cref="AttachExitHandler(global::System.Diagnostics.Process, Func{int})"/>.</returns>
-		async Task<Task<int>> AttachExitHandlerBeforeLaunch(global::System.Diagnostics.Process handle, Task startupTask)
-		{
-			var id = -1;
-			var result = AttachExitHandler(handle, () => id);
-			await startupTask;
-			id = handle.Id;
-			return result;
-		}
-
-		/// <summary>
-		/// Attach an asychronous exit handler to a given process <paramref name="handle"/>.
-		/// </summary>
-		/// <param name="handle">The <see cref="global::System.Diagnostics.Process"/> to attach an exit handler to.</param>
-		/// <param name="idProvider">A <see cref="Func{TResult}"/> that can be called to get the <see cref="global::System.Diagnostics.Process.Id"/> safely.</param>
-		/// <returns>A <see cref="Task{TResult}"/> that completes with the exit code of the process represented by <paramref name="handle"/>.</returns>
-		Task<int> AttachExitHandler(global::System.Diagnostics.Process handle, Func<int> idProvider)
-		{
-			handle.EnableRaisingEvents = true;
-
-			var tcs = new TaskCompletionSource<int>();
-			void ExitHandler(object sender, EventArgs args)
-			{
-				var id = idProvider();
-				try
-				{
-					try
-					{
-						var exitCode = handle.ExitCode;
-
-						// Try because this can be invoked twice for weird reasons
-						if (tcs.TrySetResult(exitCode))
-							logger.LogTrace("PID {pid} termination event completed", id);
-						else
-							logger.LogTrace("Ignoring duplicate PID {pid} termination event", id);
-					}
-					catch (InvalidOperationException ex)
-					{
-						if (!tcs.Task.IsCompleted)
-							throw;
-
-						logger.LogTrace(ex, "Ignoring expected PID {pid} exit handler exception!", id);
-					}
-				}
-				catch (Exception ex)
-				{
-					logger.LogError(ex, "PID {pid} exit handler exception!", id);
-				}
-			}
-
-			handle.Exited += ExitHandler;
-
-			return tcs.Task;
-		}
-
 		/// <summary>
 		/// Consume the stdout/stderr streams into a <see cref="Task"/>.
 		/// </summary>
@@ -327,7 +272,6 @@ namespace Tgstation.Server.Host.System
 					processFeatures,
 					handle,
 					null,
-					AttachExitHandler(handle, () => pid),
 					null,
 					loggerFactory.CreateLogger<Process>(),
 					true);

tests/Tgstation.Server.Host.Tests/System/TestPosixSignalHandler.cs

@@ -63,7 +63,7 @@ namespace Tgstation.Server.Host.System.Tests
 				Mock.Of<IIOManager>(),
 				loggerFactory.CreateLogger<ProcessExecutor>(),
 				loggerFactory);
-			await using var subProc = await processExecutor
+			await using var subProc = processExecutor
 				.LaunchProcess(
 					"dotnet",
 					pathToSignalTestApp,

tests/Tgstation.Server.Tests/TestSystemInteraction.cs

@@ -27,7 +27,7 @@ namespace Tgstation.Server.Tests
 				Mock.Of<ILogger<ProcessExecutor>>(),
 				loggerFactory);
 
-			await using var process = await processExecutor.LaunchProcess("test." + platformIdentifier.ScriptFileExtension, ".", string.Empty, null, true, true);
+			await using var process = processExecutor.LaunchProcess("test." + platformIdentifier.ScriptFileExtension, ".", string.Empty, null, true, true);
 			using var cts = new CancellationTokenSource();
 			cts.CancelAfter(3000);
 			var exitCode = await process.Lifetime.WaitAsync(cts.Token);
@@ -55,7 +55,7 @@ namespace Tgstation.Server.Tests
 			File.Delete(tempFile);
 			try
 			{
-				await using (var process = await processExecutor.LaunchProcess("test." + platformIdentifier.ScriptFileExtension, ".", string.Empty, tempFile, true, true))
+				await using (var process = processExecutor.LaunchProcess("test." + platformIdentifier.ScriptFileExtension, ".", string.Empty, tempFile, true, true))
 				{
 					using var cts = new CancellationTokenSource();
 					cts.CancelAfter(3000);

tests/Tgstation.Server.Tests/TestVersions.cs

@@ -410,7 +410,7 @@ namespace Tgstation.Server.Tests
 
 			Assert.IsTrue(supportsCli);
 
-			await using var process = await processExecutor.LaunchProcess(
+			await using var process = processExecutor.LaunchProcess(
 				ddPath,
 				Environment.CurrentDirectory,
 				"fake.dmb -map-threads 3 -close",