Ensure child processes are cleanup up

src/WinSW.Core/Configuration/XmlServiceConfig.cs

@@ -597,7 +597,7 @@ namespace WinSW
         /// <summary>
         /// Environment variable overrides
         /// </summary>
-        public override Dictionary<string, string> EnvironmentVariables => new Dictionary<string, string>(this.environmentVariables);
+        public override Dictionary<string, string> EnvironmentVariables => this.environmentVariables;
 
         /// <summary>
         /// List of downloads to be performed by the wrapper before starting

src/WinSW.Core/Util/ProcessExtensions.cs

@@ -0,0 +1,111 @@
+using System;
+using System.Collections.Generic;
+using System.ComponentModel;
+using System.Diagnostics;
+using log4net;
+using static WinSW.Native.ProcessApis;
+
+namespace WinSW.Util
+{
+    public static class ProcessExtensions
+    {
+        private static readonly ILog Logger = LogManager.GetLogger(typeof(ProcessExtensions));
+
+        public static void StopTree(this Process process, TimeSpan stopTimeout)
+        {
+            Stop(process, stopTimeout);
+
+            foreach (Process child in GetDescendants(process))
+            {
+                StopTree(child, stopTimeout);
+            }
+        }
+
+        internal static void StopDescendants(this Process process, TimeSpan stopTimeout)
+        {
+            foreach (Process child in GetDescendants(process))
+            {
+                StopTree(child, stopTimeout);
+            }
+        }
+
+        private static void Stop(Process process, TimeSpan stopTimeout)
+        {
+            Logger.Info("Stopping process " + process.Id);
+
+            if (process.HasExited)
+            {
+                Logger.Info("Process " + process.Id + " is already stopped");
+                return;
+            }
+
+            // (bool sent, bool exited)
+            KeyValuePair<bool, bool> result = SignalHelper.SendCtrlCToProcess(process, stopTimeout);
+            bool exited = result.Value;
+            if (!exited)
+            {
+                bool sent = result.Key;
+                if (sent)
+                {
+                    Logger.Info("Process " + process.Id + " did not respond to Ctrl+C signal - Killing as fallback");
+                }
+
+                try
+                {
+                    process.Kill();
+                }
+                catch when (process.HasExited)
+                {
+                }
+            }
+
+            // TODO: Propagate error if process kill fails? Currently we use the legacy behavior
+        }
+
+        private static unsafe List<Process> GetDescendants(Process root)
+        {
+            DateTime startTime = root.StartTime;
+            int processId = root.Id;
+
+            var children = new List<Process>();
+
+            foreach (Process process in Process.GetProcesses())
+            {
+                try
+                {
+                    if (process.StartTime <= startTime)
+                    {
+                        goto Next;
+                    }
+
+                    IntPtr handle = process.Handle;
+
+                    if (NtQueryInformationProcess(
+                        handle,
+                        PROCESSINFOCLASS.ProcessBasicInformation,
+                        out PROCESS_BASIC_INFORMATION information,
+                        sizeof(PROCESS_BASIC_INFORMATION)) != 0)
+                    {
+                        goto Next;
+                    }
+
+                    if ((int)information.InheritedFromUniqueProcessId == processId)
+                    {
+                        Logger.Info($"Found child process '{process.Format()}'.");
+                        children.Add(process);
+                        continue;
+                    }
+
+                Next:
+                    process.Dispose();
+                }
+                catch (Exception e) when (e is InvalidOperationException || e is Win32Exception)
+                {
+                    process.Dispose();
+                }
+            }
+
+            return children;
+        }
+    }
+}

src/WinSW.Core/Util/ProcessHelper.cs

@@ -1,197 +0,0 @@
-using System;
-using System.Collections.Generic;
-using System.ComponentModel;
-using System.Diagnostics;
-using log4net;
-using static WinSW.Native.ProcessApis;
-
-namespace WinSW.Util
-{
-    /// <summary>
-    /// Provides helper classes for Process Management
-    /// </summary>
-    /// <remarks>Since WinSW 2.0</remarks>
-    public class ProcessHelper
-    {
-        private static readonly ILog Logger = LogManager.GetLogger(typeof(ProcessHelper));
-
-        /// <summary>
-        /// Gets all children of the specified process.
-        /// </summary>
-        /// <param name="processId">Process PID</param>
-        /// <returns>List of child process PIDs</returns>
-        private static unsafe List<Process> GetChildProcesses(DateTime startTime, int processId)
-        {
-            var children = new List<Process>();
-
-            foreach (Process process in Process.GetProcesses())
-            {
-                try
-                {
-                    if (process.StartTime <= startTime)
-                    {
-                        goto Next;
-                    }
-
-                    IntPtr handle = process.Handle;
-
-                    if (NtQueryInformationProcess(
-                        handle,
-                        PROCESSINFOCLASS.ProcessBasicInformation,
-                        out PROCESS_BASIC_INFORMATION information,
-                        sizeof(PROCESS_BASIC_INFORMATION)) != 0)
-                    {
-                        goto Next;
-                    }
-
-                    if ((int)information.InheritedFromUniqueProcessId == processId)
-                    {
-                        Logger.Info($"Found child process '{process.Format()}'.");
-                        children.Add(process);
-                        continue;
-                    }
-
-                Next:
-                    process.Dispose();
-                }
-                catch (Exception e) when (e is InvalidOperationException || e is Win32Exception)
-                {
-                    process.Dispose();
-                }
-            }
-
-            return children;
-        }
-
-        /// <summary>
-        /// Stops the process.
-        /// If the process cannot be stopped within the stop timeout, it gets killed
-        /// </summary>
-        public static void StopProcess(Process process, TimeSpan stopTimeout)
-        {
-            Logger.Info("Stopping process " + process.Id);
-
-            if (process.HasExited)
-            {
-                Logger.Info("Process " + process.Id + " is already stopped");
-                return;
-            }
-
-            // (bool sent, bool exited)
-            KeyValuePair<bool, bool> result = SignalHelper.SendCtrlCToProcess(process, stopTimeout);
-            bool exited = result.Value;
-            if (!exited)
-            {
-                bool sent = result.Key;
-                if (sent)
-                {
-                    Logger.Warn("Process " + process.Id + " did not respond to Ctrl+C signal - Killing as fallback");
-                }
-
-                try
-                {
-                    process.Kill();
-                }
-                catch when (process.HasExited)
-                {
-                }
-            }
-
-            // TODO: Propagate error if process kill fails? Currently we use the legacy behavior
-        }
-
-        /// <summary>
-        /// Terminate process and its children.
-        /// By default the child processes get terminated first.
-        /// </summary>
-        public static void StopProcessTree(Process process, TimeSpan stopTimeout)
-        {
-            StopProcess(process, stopTimeout);
-
-            foreach (Process child in GetChildProcesses(process.StartTime, process.Id))
-            {
-                StopProcessTree(child, stopTimeout);
-            }
-        }
-
-        /// <summary>
-        /// Starts a process and asynchronosly waits for its termination.
-        /// Once the process exits, the callback will be invoked.
-        /// </summary>
-        /// <param name="processToStart">Process object to be used</param>
-        /// <param name="executable">Executable, which should be invoked</param>
-        /// <param name="arguments">Arguments to be passed</param>
-        /// <param name="envVars">Additional environment variables</param>
-        /// <param name="workingDirectory">Working directory</param>
-        /// <param name="priority">Priority</param>
-        /// <param name="onExited">Completion callback. If null, the completion won't be monitored</param>
-        /// <param name="logHandler">Log handler. If enabled, logs will be redirected to the process and then reported</param>
-        public static void StartProcessAndCallbackForExit(
-            Process processToStart,
-            string? executable = null,
-            string? arguments = null,
-            Dictionary<string, string>? envVars = null,
-            string? workingDirectory = null,
-            ProcessPriorityClass? priority = null,
-            Action<Process>? onExited = null,
-            LogHandler? logHandler = null,
-            bool hideWindow = false)
-        {
-            var ps = processToStart.StartInfo;
-            ps.FileName = executable ?? ps.FileName;
-            ps.Arguments = arguments ?? ps.Arguments;
-            ps.WorkingDirectory = workingDirectory ?? ps.WorkingDirectory;
-            ps.CreateNoWindow = hideWindow;
-            ps.UseShellExecute = false;
-            ps.RedirectStandardOutput = logHandler != null;
-            ps.RedirectStandardError = logHandler != null;
-
-            if (envVars != null)
-            {
-                var newEnvironment =
-#if NETCOREAPP
-                    ps.Environment;
-#else
-                    ps.EnvironmentVariables;
-#endif
-                foreach (KeyValuePair<string, string> pair in envVars)
-                {
-                    newEnvironment[pair.Key] = pair.Value;
-                }
-            }
-
-            processToStart.Start();
-            Logger.Info("Started process " + processToStart.Id);
-
-            if (priority != null)
-            {
-                processToStart.PriorityClass = priority.Value;
-            }
-
-            // Redirect logs if required
-            if (logHandler != null)
-            {
-                Logger.Debug("Forwarding logs of the process " + processToStart + " to " + logHandler);
-                logHandler.Log(processToStart.StandardOutput, processToStart.StandardError);
-            }
-
-            // monitor the completion of the process
-            if (onExited != null)
-            {
-                processToStart.Exited += (sender, _) =>
-                {
-                    try
-                    {
-                        onExited((Process)sender!);
-                    }
-                    catch (Exception e)
-                    {
-                        Logger.Error("Unhandled exception in event handler.", e);
-                    }
-                };
-
-                processToStart.EnableRaisingEvents = true;
-            }
-        }
-    }
-}

src/WinSW.Plugins/RunawayProcessKillerExtension.cs

@@ -270,7 +270,7 @@ namespace WinSW.Plugins.RunawayProcessKiller
             bldr.Append(proc);
 
             Logger.Warn(bldr.ToString());
-            ProcessHelper.StopProcessTree(proc, this.StopTimeout);
+            proc.StopTree(this.StopTimeout);
         }
 
         /// <summary>

src/WinSW.Tests/Extensions/RunawayProcessKillerTest.cs

@@ -107,7 +107,7 @@ $@"<service>
                     if (!proc.HasExited)
                     {
                         Console.Error.WriteLine("Test: Killing runaway process with ID=" + proc.Id);
-                        ProcessHelper.StopProcessTree(proc, TimeSpan.FromMilliseconds(100));
+                        proc.StopTree(TimeSpan.FromMilliseconds(100));
                         if (!proc.HasExited)
                         {
                             // The test is failed here anyway, but we add additional diagnostics info

src/WinSW/WrapperService.cs

@@ -16,11 +16,9 @@ namespace WinSW
 {
     public sealed class WrapperService : ServiceBase, IEventLogger, IServiceEventLog
     {
-        private readonly Process process = new Process();
+        internal static readonly WrapperServiceEventLogProvider eventLogProvider = new WrapperServiceEventLogProvider();
-        private readonly XmlServiceConfig config;
-        private Dictionary<string, string>? envs;
 
-        internal WinSWExtensionManager ExtensionManager { get; }
+        private static readonly TimeSpan additionalStopTimeout = new TimeSpan(TimeSpan.TicksPerSecond);
 
         private static readonly ILog Log = LogManager.GetLogger(
 #if NETCOREAPP
@@ -28,13 +26,18 @@ namespace WinSW
 #endif
             "WinSW");
 
-        internal static readonly WrapperServiceEventLogProvider eventLogProvider = new WrapperServiceEventLogProvider();
+        private readonly XmlServiceConfig config;
+
+        private Process process = null!;
+        private volatile Process? poststartProcess;
+
+        internal WinSWExtensionManager ExtensionManager { get; }
 
         /// <summary>
         /// Indicates to the watch dog thread that we are going to terminate the process,
         /// so don't try to kill us when the child exits.
         /// </summary>
-        private bool orderlyShutdown;
+        private volatile bool orderlyShutdown;
         private bool shuttingdown;
 
         /// <summary>
@@ -243,8 +246,6 @@ namespace WinSW
 
         private void DoStart()
         {
-            this.envs = this.config.EnvironmentVariables;
-
             this.HandleFileCopies();
 
             // handle downloads
@@ -285,20 +286,21 @@ namespace WinSW
                 throw new AggregateException(exceptions);
             }
 
-            try
-            {
             string? prestartExecutable = this.config.PrestartExecutable;
             if (prestartExecutable != null)
+            {
+                try
                 {
                     using Process process = this.StartProcess(prestartExecutable, this.config.PrestartArguments);
                     this.WaitForProcessToExit(process);
                     this.LogInfo($"Pre-start process '{process.Format()}' exited with code {process.ExitCode}.");
-                }
+                    process.StopDescendants(additionalStopTimeout);
                 }
                 catch (Exception e)
                 {
                     Log.Error(e);
                 }
+            }
 
             string startArguments = this.config.StartArguments ?? this.config.Arguments;
 
@@ -309,7 +311,7 @@ namespace WinSW
             this.ExtensionManager.FireOnWrapperStarted();
 
             LogHandler executableLogHandler = this.CreateExecutableLogHandler();
-            this.StartProcess(this.process, startArguments, this.config.Executable, executableLogHandler);
+            this.process = this.StartProcess(this.config.Executable, startArguments, this.OnMainProcessExited, executableLogHandler);
             this.ExtensionManager.FireOnProcessStarted(this.process);
 
             try
@@ -317,10 +319,19 @@ namespace WinSW
                 string? poststartExecutable = this.config.PoststartExecutable;
                 if (poststartExecutable != null)
                 {
-                    using Process process = this.StartProcess(poststartExecutable, this.config.PoststartArguments, process =>
+                    using Process process = StartProcessLocked();
-                    {
+                    this.WaitForProcessToExit(process);
                     this.LogInfo($"Post-start process '{process.Format()}' exited with code {process.ExitCode}.");
-                    });
+                    process.StopDescendants(additionalStopTimeout);
+                    this.poststartProcess = null;
+
+                    Process StartProcessLocked()
+                    {
+                        lock (this)
+                        {
+                            return this.poststartProcess = this.StartProcess(poststartExecutable, this.config.PoststartArguments);
+                        }
+                    }
                 }
             }
             catch (Exception e)
@@ -333,63 +344,75 @@ namespace WinSW
         /// Called when we are told by Windows SCM to exit.
         /// </summary>
         private void DoStop()
-        {
-            try
         {
             string? prestopExecutable = this.config.PrestopExecutable;
             if (prestopExecutable != null)
+            {
+                try
                 {
                     using Process process = this.StartProcess(prestopExecutable, this.config.PrestopArguments);
                     this.WaitForProcessToExit(process);
                     this.LogInfo($"Pre-stop process '{process.Format()}' exited with code {process.ExitCode}.");
-                }
+                    process.StopDescendants(additionalStopTimeout);
                 }
                 catch (Exception e)
                 {
                     Log.Error(e);
                 }
+            }
 
             string? stopArguments = this.config.StopArguments;
             this.LogInfo("Stopping " + this.config.Id);
             this.orderlyShutdown = true;
-            this.process.EnableRaisingEvents = false;
 
             if (stopArguments is null)
             {
                 Log.Debug("ProcessKill " + this.process.Id);
-                ProcessHelper.StopProcessTree(this.process, this.config.StopTimeout);
+                this.process.StopTree(this.config.StopTimeout);
                 this.ExtensionManager.FireOnProcessTerminated(this.process);
             }
             else
             {
                 this.SignalPending();
 
-                Process stopProcess = new Process();
-
                 string stopExecutable = this.config.StopExecutable ?? this.config.Executable;
 
-                // TODO: Redirect logging to Log4Net once https://github.com/kohsuke/winsw/pull/213 is integrated
-                this.StartProcess(stopProcess, stopArguments, stopExecutable, null);
-
-                Log.Debug("WaitForProcessToExit " + this.process.Id + "+" + stopProcess.Id);
-                this.WaitForProcessToExit(this.process);
-                this.WaitForProcessToExit(stopProcess);
-            }
-
                 try
                 {
+                    // TODO: Redirect logging to Log4Net once https://github.com/kohsuke/winsw/pull/213 is integrated
+                    Process stopProcess = this.StartProcess(stopExecutable, stopArguments);
+
+                    Log.Debug("WaitForProcessToExit " + this.process.Id + "+" + stopProcess.Id);
+                    this.WaitForProcessToExit(stopProcess);
+                    stopProcess.StopDescendants(additionalStopTimeout);
+
+                    this.WaitForProcessToExit(this.process);
+                    this.process.StopDescendants(this.config.StopTimeout);
+                }
+                catch
+                {
+                    this.process.StopTree(this.config.StopTimeout);
+                    throw;
+                }
+            }
+
+            this.poststartProcess?.StopTree(additionalStopTimeout);
+
             string? poststopExecutable = this.config.PoststopExecutable;
             if (poststopExecutable != null)
+            {
+                try
                 {
                     using Process process = this.StartProcess(poststopExecutable, this.config.PoststopArguments);
                     this.WaitForProcessToExit(process);
                     this.LogInfo($"Post-stop process '{process.Format()}' exited with code {process.ExitCode}.");
-                }
+                    process.StopDescendants(additionalStopTimeout);
                 }
                 catch (Exception e)
                 {
                     Log.Error(e);
                 }
+            }
 
             // Stop extensions
             this.ExtensionManager.FireBeforeWrapperStopped();
@@ -447,10 +470,7 @@ namespace WinSW
             sc.SetStatus(this.ServiceHandle, ServiceControllerStatus.Stopped);
         }
 
-        private void StartProcess(Process processToStart, string arguments, string executable, LogHandler? logHandler)
+        private void OnMainProcessExited(Process process)
-        {
-            // Define handler of the completed process
-            void OnProcessCompleted(Process process)
         {
             string display = process.Format();
 
@@ -462,45 +482,71 @@ namespace WinSW
             {
                 Log.Warn($"Child process '{display}' finished with code {process.ExitCode}.");
 
+                process.StopDescendants(this.config.StopTimeout);
+
+                lock (this)
+                {
+                    this.poststartProcess?.StopTree(new TimeSpan(TimeSpan.TicksPerMillisecond));
+                }
+
                 // if we finished orderly, report that to SCM.
                 // by not reporting unclean shutdown, we let Windows SCM to decide if it wants to
                 // restart the service automatically
-                    try
-                    {
                 if (process.ExitCode == 0)
+                {
+                    try
                     {
                         this.SignalStopped();
                     }
-                    }
+                    catch (Exception e)
-                    finally
                     {
+                        Log.Error(e);
+                    }
+                }
+
                 Environment.Exit(process.ExitCode);
             }
         }
-            }
 
-            // Invoke process and exit
+        private Process StartProcess(string executable, string? arguments, Action<Process>? onExited = null, LogHandler? logHandler = null)
-            ProcessHelper.StartProcessAndCallbackForExit(
-                processToStart: processToStart,
-                executable: executable,
-                arguments: arguments,
-                envVars: this.envs,
-                workingDirectory: this.config.WorkingDirectory,
-                priority: this.config.Priority,
-                onExited: OnProcessCompleted,
-                logHandler: logHandler,
-                hideWindow: this.config.HideWindow);
-        }
-
-        private Process StartProcess(string executable, string? arguments, Action<Process>? onExited = null)
         {
-            var info = new ProcessStartInfo(executable, arguments)
+            var startInfo = new ProcessStartInfo(executable, arguments)
             {
                 UseShellExecute = false,
                 WorkingDirectory = this.config.WorkingDirectory,
+                CreateNoWindow = this.config.HideWindow,
+                RedirectStandardOutput = logHandler != null,
+                RedirectStandardError = logHandler != null,
             };
 
-            Process process = Process.Start(info);
+            Dictionary<string, string> environment = this.config.EnvironmentVariables;
+            if (environment.Count > 0)
+            {
+                var newEnvironment =
+#if NETCOREAPP
+                    startInfo.Environment;
+#else
+                    startInfo.EnvironmentVariables;
+#endif
+                foreach (KeyValuePair<string, string> pair in environment)
+                {
+                    newEnvironment[pair.Key] = pair.Value;
+                }
+            }
+
+            Process process = Process.Start(startInfo);
+
+            Log.Info($"Started process {process.Format()}.");
+
+            if (this.config.Priority is ProcessPriorityClass priority)
+            {
+                process.PriorityClass = priority;
+            }
+
+            if (logHandler != null)
+            {
+                logHandler.Log(process.StandardOutput, process.StandardError);
+            }
 
             if (onExited != null)
             {