From bd61d2986f0dcecffdd8c19dea7a9f5772e08bcc Mon Sep 17 00:00:00 2001
From: NextTurn <45985406+NextTurn@users.noreply.github.com>
Date: Sat, 1 Aug 2020 00:00:00 +0800
Subject: [PATCH] Ensure child processes are cleanup up
---
.../Configuration/XmlServiceConfig.cs | 2 +-
src/WinSW.Core/Util/ProcessExtensions.cs | 111 ++++++++++
src/WinSW.Core/Util/ProcessHelper.cs | 197 ------------------
.../RunawayProcessKillerExtension.cs | 2 +-
.../Extensions/RunawayProcessKillerTest.cs | 2 +-
src/WinSW/WrapperService.cs | 196 ++++++++++-------
6 files changed, 235 insertions(+), 275 deletions(-)
create mode 100644 src/WinSW.Core/Util/ProcessExtensions.cs
delete mode 100644 src/WinSW.Core/Util/ProcessHelper.cs
diff --git a/src/WinSW.Core/Configuration/XmlServiceConfig.cs b/src/WinSW.Core/Configuration/XmlServiceConfig.cs
index 2656f21..0e7a9ff 100644
--- a/src/WinSW.Core/Configuration/XmlServiceConfig.cs
+++ b/src/WinSW.Core/Configuration/XmlServiceConfig.cs
@@ -597,7 +597,7 @@ namespace WinSW
///
/// Environment variable overrides
///
- public override Dictionary EnvironmentVariables => new Dictionary(this.environmentVariables);
+ public override Dictionary EnvironmentVariables => this.environmentVariables;
///
/// List of downloads to be performed by the wrapper before starting
diff --git a/src/WinSW.Core/Util/ProcessExtensions.cs b/src/WinSW.Core/Util/ProcessExtensions.cs
new file mode 100644
index 0000000..f76a369
--- /dev/null
+++ b/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 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 GetDescendants(Process root)
+ {
+ DateTime startTime = root.StartTime;
+ int processId = root.Id;
+
+ var children = new List();
+
+ 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;
+ }
+ }
+}
diff --git a/src/WinSW.Core/Util/ProcessHelper.cs b/src/WinSW.Core/Util/ProcessHelper.cs
deleted file mode 100644
index e4ca9db..0000000
--- a/src/WinSW.Core/Util/ProcessHelper.cs
+++ /dev/null
@@ -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
-{
- ///
- /// Provides helper classes for Process Management
- ///
- /// Since WinSW 2.0
- public class ProcessHelper
- {
- private static readonly ILog Logger = LogManager.GetLogger(typeof(ProcessHelper));
-
- ///
- /// Gets all children of the specified process.
- ///
- /// Process PID
- /// List of child process PIDs
- private static unsafe List GetChildProcesses(DateTime startTime, int processId)
- {
- var children = new List();
-
- 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;
- }
-
- ///
- /// Stops the process.
- /// If the process cannot be stopped within the stop timeout, it gets killed
- ///
- 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 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
- }
-
- ///
- /// Terminate process and its children.
- /// By default the child processes get terminated first.
- ///
- public static void StopProcessTree(Process process, TimeSpan stopTimeout)
- {
- StopProcess(process, stopTimeout);
-
- foreach (Process child in GetChildProcesses(process.StartTime, process.Id))
- {
- StopProcessTree(child, stopTimeout);
- }
- }
-
- ///
- /// Starts a process and asynchronosly waits for its termination.
- /// Once the process exits, the callback will be invoked.
- ///
- /// Process object to be used
- /// Executable, which should be invoked
- /// Arguments to be passed
- /// Additional environment variables
- /// Working directory
- /// Priority
- /// Completion callback. If null, the completion won't be monitored
- /// Log handler. If enabled, logs will be redirected to the process and then reported
- public static void StartProcessAndCallbackForExit(
- Process processToStart,
- string? executable = null,
- string? arguments = null,
- Dictionary? envVars = null,
- string? workingDirectory = null,
- ProcessPriorityClass? priority = null,
- Action? 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 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;
- }
- }
- }
-}
diff --git a/src/WinSW.Plugins/RunawayProcessKillerExtension.cs b/src/WinSW.Plugins/RunawayProcessKillerExtension.cs
index 474a379..a18ca14 100644
--- a/src/WinSW.Plugins/RunawayProcessKillerExtension.cs
+++ b/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);
}
///
diff --git a/src/WinSW.Tests/Extensions/RunawayProcessKillerTest.cs b/src/WinSW.Tests/Extensions/RunawayProcessKillerTest.cs
index 6316e02..daee8b5 100644
--- a/src/WinSW.Tests/Extensions/RunawayProcessKillerTest.cs
+++ b/src/WinSW.Tests/Extensions/RunawayProcessKillerTest.cs
@@ -107,7 +107,7 @@ $@"
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
diff --git a/src/WinSW/WrapperService.cs b/src/WinSW/WrapperService.cs
index 8594736..ae71a5e 100644
--- a/src/WinSW/WrapperService.cs
+++ b/src/WinSW/WrapperService.cs
@@ -16,11 +16,9 @@ namespace WinSW
{
public sealed class WrapperService : ServiceBase, IEventLogger, IServiceEventLog
{
- private readonly Process process = new Process();
- private readonly XmlServiceConfig config;
- private Dictionary? envs;
+ internal static readonly WrapperServiceEventLogProvider eventLogProvider = new WrapperServiceEventLogProvider();
- 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; }
///
/// 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.
///
- private bool orderlyShutdown;
+ private volatile bool orderlyShutdown;
private bool shuttingdown;
///
@@ -243,8 +246,6 @@ namespace WinSW
private void DoStart()
{
- this.envs = this.config.EnvironmentVariables;
-
this.HandleFileCopies();
// handle downloads
@@ -285,19 +286,20 @@ namespace WinSW
throw new AggregateException(exceptions);
}
- try
+ string? prestartExecutable = this.config.PrestartExecutable;
+ if (prestartExecutable != null)
{
- 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);
}
- }
- 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()
{
- this.LogInfo($"Post-start process '{process.Format()}' exited with code {process.ExitCode}.");
- });
+ lock (this)
+ {
+ return this.poststartProcess = this.StartProcess(poststartExecutable, this.config.PoststartArguments);
+ }
+ }
}
}
catch (Exception e)
@@ -334,61 +345,73 @@ namespace WinSW
///
private void DoStop()
{
- try
+ string? prestopExecutable = this.config.PrestopExecutable;
+ if (prestopExecutable != null)
{
- 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);
}
- }
- 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);
+ 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(this.process);
- this.WaitForProcessToExit(stopProcess);
+ 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;
+ }
}
- try
+ this.poststartProcess?.StopTree(additionalStopTimeout);
+
+ string? poststopExecutable = this.config.PoststopExecutable;
+ if (poststopExecutable != null)
{
- 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);
}
- }
- catch (Exception e)
- {
- Log.Error(e);
}
// Stop extensions
@@ -447,60 +470,83 @@ 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();
+
+ if (this.orderlyShutdown)
{
- string display = process.Format();
+ this.LogInfo($"Child process '{display}' terminated with code {process.ExitCode}.");
+ }
+ else
+ {
+ Log.Warn($"Child process '{display}' finished with code {process.ExitCode}.");
- if (this.orderlyShutdown)
+ process.StopDescendants(this.config.StopTimeout);
+
+ lock (this)
{
- this.LogInfo($"Child process '{display}' terminated with code {process.ExitCode}.");
+ this.poststartProcess?.StopTree(new TimeSpan(TimeSpan.TicksPerMillisecond));
}
- else
- {
- Log.Warn($"Child process '{display}' finished with code {process.ExitCode}.");
- // 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
+ // 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
+ if (process.ExitCode == 0)
+ {
try
{
- if (process.ExitCode == 0)
- {
- this.SignalStopped();
- }
+ this.SignalStopped();
}
- finally
+ catch (Exception e)
{
- Environment.Exit(process.ExitCode);
+ Log.Error(e);
}
}
- }
- // Invoke process and exit
- 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);
+ Environment.Exit(process.ExitCode);
+ }
}
- private Process StartProcess(string executable, string? arguments, Action? onExited = null)
+ private Process StartProcess(string executable, string? arguments, Action? onExited = null, LogHandler? logHandler = 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 environment = this.config.EnvironmentVariables;
+ if (environment.Count > 0)
+ {
+ var newEnvironment =
+#if NETCOREAPP
+ startInfo.Environment;
+#else
+ startInfo.EnvironmentVariables;
+#endif
+ foreach (KeyValuePair 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)
{