diff --git a/src/Core/ServiceWrapper/Main.cs b/src/Core/ServiceWrapper/Main.cs index be83d5e..c6ce65f 100644 --- a/src/Core/ServiceWrapper/Main.cs +++ b/src/Core/ServiceWrapper/Main.cs @@ -133,25 +133,6 @@ namespace winsw } } - /// - /// Starts a thread that protects the execution with a try/catch block. - /// It appears that in .NET, unhandled exception in any thread causes the app to terminate - /// http://msdn.microsoft.com/en-us/library/ms228965.aspx - /// - private void StartThread(ThreadStart main) - { - new Thread(delegate() { - try - { - main(); - } - catch (Exception e) - { - Log.Error("Thread failed unexpectedly",e); - } - }).Start(); - } - /// /// Handle the creation of the logfiles based on the optional logmode setting. /// @@ -419,53 +400,26 @@ namespace winsw private void StartProcess(Process processToStart, string arguments, String executable) { - var ps = processToStart.StartInfo; - ps.FileName = executable; - ps.Arguments = arguments; - ps.WorkingDirectory = _descriptor.WorkingDirectory; - ps.CreateNoWindow = false; - ps.UseShellExecute = false; - ps.RedirectStandardInput = true; // this creates a pipe for stdin to the new process, instead of having it inherit our stdin. - ps.RedirectStandardOutput = true; - ps.RedirectStandardError = true; - - foreach (string key in _envs.Keys) - { - Environment.SetEnvironmentVariable(key, _envs[key]); - // ps.EnvironmentVariables[key] = envs[key]; // bugged (lower cases all variable names due to StringDictionary being used, see http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=326163) - } - - // TODO: Make it generic via extension points. The issue mentioned above should be ideally worked around somehow - ps.EnvironmentVariables[WinSWSystem.ENVVAR_NAME_SERVICE_ID.ToLower()] = _descriptor.Id; - - processToStart.Start(); - Log.Info("Started " + processToStart.Id); - - var priority = _descriptor.Priority; - if (priority != ProcessPriorityClass.Normal) - processToStart.PriorityClass = priority; - - // monitor the completion of the process - StartThread(delegate + + // Define handler of the completed process + ProcessCompletionCallback processCompletionCallback = delegate(Process proc) { string msg = processToStart.Id + " - " + processToStart.StartInfo.FileName + " " + processToStart.StartInfo.Arguments; - processToStart.WaitForExit(); - try { if (_orderlyShutdown) { - LogEvent("Child process [" + msg + "] terminated with " + processToStart.ExitCode, EventLogEntryType.Information); + LogEvent("Child process [" + msg + "] terminated with " + proc.ExitCode, EventLogEntryType.Information); } else { - LogEvent("Child process [" + msg + "] finished with " + processToStart.ExitCode, EventLogEntryType.Warning); + LogEvent("Child process [" + msg + "] finished with " + proc.ExitCode, EventLogEntryType.Warning); // 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 (processToStart.ExitCode == 0) + if (proc.ExitCode == 0) SignalShutdownComplete(); - Environment.Exit(processToStart.ExitCode); + Environment.Exit(proc.ExitCode); } } catch (InvalidOperationException ioe) @@ -475,13 +429,23 @@ namespace winsw try { - processToStart.Dispose(); + proc.Dispose(); } catch (InvalidOperationException ioe) { LogEvent("Dispose " + ioe.Message); } - }); + }; + + // Invoke process and exit + ProcessHelper.StartProcessAndCallbackForExit( + processToStart: processToStart, + executable: executable, + arguments: arguments, + envVars: _envs, + workingDirectory: _descriptor.WorkingDirectory, + priority: _descriptor.Priority, + callback: processCompletionCallback); } public static int Main(string[] args) diff --git a/src/Core/WinSWCore/Util/ProcessHelper.cs b/src/Core/WinSWCore/Util/ProcessHelper.cs index e26cf55..03327aa 100644 --- a/src/Core/WinSWCore/Util/ProcessHelper.cs +++ b/src/Core/WinSWCore/Util/ProcessHelper.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Management; using System.Text; +using System.Threading; namespace winsw.Util { @@ -117,6 +118,79 @@ namespace winsw.Util } } - // TODO: Also move StartProcess methods once LogEvent()/WriteEvent() mess gets solved + /// + /// Starts a process and asynchronosly waits for its termination. + /// Once the process exits, the callback will be invoked. + /// + /// Process object to be used + /// Arguments to be passed + /// Executable, which should be invoked + /// Additional environment variables + /// Working directory + /// Priority + /// Completion callback. If null, the completion won't be monitored + public static void StartProcessAndCallbackForExit(Process processToStart, String executable = null, string arguments = null, Dictionary envVars = null, + string workingDirectory = null, ProcessPriorityClass? priority = null, ProcessCompletionCallback callback = null) + { + var ps = processToStart.StartInfo; + ps.FileName = executable ?? ps.FileName; + ps.Arguments = arguments ?? ps.Arguments; + ps.WorkingDirectory = workingDirectory ?? ps.WorkingDirectory; + ps.CreateNoWindow = false; + ps.UseShellExecute = false; + ps.RedirectStandardInput = true; // this creates a pipe for stdin to the new process, instead of having it inherit our stdin. + ps.RedirectStandardOutput = true; + ps.RedirectStandardError = true; + + if (envVars != null) + { + foreach (string key in envVars.Keys) + { + Environment.SetEnvironmentVariable(key, envVars[key]); + // DONTDO: ps.EnvironmentVariables[key] = envs[key]; + // bugged (lower cases all variable names due to StringDictionary being used, see http://connect.microsoft.com/VisualStudio/feedback/ViewFeedback.aspx?FeedbackID=326163) + } + } + + processToStart.Start(); + Logger.Info("Started process " + processToStart.Id); + + if (priority != null && priority.Value != ProcessPriorityClass.Normal) + { + processToStart.PriorityClass = priority.Value; + } + + // monitor the completion of the process + if (callback != null) + { + StartThread(delegate + { + processToStart.WaitForExit(); + callback(processToStart); + }); + } + } + + /// + /// Starts a thread that protects the execution with a try/catch block. + /// It appears that in .NET, unhandled exception in any thread causes the app to terminate + /// http://msdn.microsoft.com/en-us/library/ms228965.aspx + /// + public static void StartThread(ThreadStart main) + { + new Thread(delegate() + { + try + { + main(); + } + catch (Exception e) + { + Logger.Error("Thread failed unexpectedly", e); + } + }).Start(); + } } + + public delegate void ProcessCompletionCallback(Process process); } diff --git a/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs b/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs index 0677549..66d7e0f 100644 --- a/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs +++ b/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs @@ -27,6 +27,12 @@ namespace winsw.Plugins.RunawayProcessKiller /// public bool StopParentProcessFirst { get; private set; } + /// + /// If true, the runaway process will be checked for the WinSW environment variable before termination. + /// This option is not documented AND not supposed to be used by users. + /// + public bool CheckWinSWEnvironmentVariable { get; private set; } + public override String DisplayName { get { return "Runaway Process Killer"; } } private String ServiceId { get; set; } @@ -38,9 +44,12 @@ namespace winsw.Plugins.RunawayProcessKiller // Default initializer } - public RunawayProcessKillerExtension(String pidfile) + public RunawayProcessKillerExtension(String pidfile, int stopTimeoutMs = 5000, bool stopParentFirst = false, bool checkWinSWEnvironmentVariable = true) { this.Pidfile = pidfile; + this.StopTimeout = TimeSpan.FromMilliseconds(5000); + this.StopParentProcessFirst = stopParentFirst; + this.CheckWinSWEnvironmentVariable = checkWinSWEnvironmentVariable; } public override void Configure(ServiceDescriptor descriptor, XmlNode node) @@ -51,6 +60,9 @@ namespace winsw.Plugins.RunawayProcessKiller StopTimeout = TimeSpan.FromMilliseconds(Int32.Parse(XmlHelper.SingleElement(node, "stopTimeout", false))); StopParentProcessFirst = Boolean.Parse(XmlHelper.SingleElement(node, "stopParentFirst", false)); ServiceId = descriptor.Id; + //TODO: Consider making it documented + var checkWinSWEnvironmentVariable = XmlHelper.SingleElement(node, "checkWinSWEnvironmentVariable", true); + CheckWinSWEnvironmentVariable = checkWinSWEnvironmentVariable != null ? Boolean.Parse(checkWinSWEnvironmentVariable) : true; } /// @@ -89,6 +101,7 @@ namespace winsw.Plugins.RunawayProcessKiller } // Now check the process + Logger.DebugFormat("Checking the potentially runaway process with PID={0}", pid); Process proc; try { @@ -105,25 +118,32 @@ namespace winsw.Plugins.RunawayProcessKiller // TODO: This method is not ideal since it works only for vars explicitly mentioned in the start info // No Windows 10- compatible solution for EnvVars retrieval, see https://blog.gapotchenko.com/eazfuscator.net/reading-environment-variables StringDictionary previousProcessEnvVars = proc.StartInfo.EnvironmentVariables; - String expectedEnvVarName = WinSWSystem.ENVVAR_NAME_SERVICE_ID.ToLower(); + String expectedEnvVarName = WinSWSystem.ENVVAR_NAME_SERVICE_ID; if (previousProcessEnvVars.ContainsKey(expectedEnvVarName)) { + // StringDictionary is case-insensitive, hence it will fetch variable definitions in any case affiliatedServiceId = previousProcessEnvVars[expectedEnvVarName]; } - else + else if (CheckWinSWEnvironmentVariable) { Logger.Warn("The process " + pid + " has no " + expectedEnvVarName + " environment variable defined. " - + "The process has not been started by this service, hence it won't be terminated."); + + "The process has not been started by WinSW, hence it won't be terminated."); if (Logger.IsDebugEnabled) { - foreach (string key in previousProcessEnvVars.Keys) { - Logger.Debug("Env var of " + pid + ": " + key + "=" + previousProcessEnvVars[key]); - } + //TODO replace by String.Join() in .NET 4 + String[] keys = new String[previousProcessEnvVars.Count]; + previousProcessEnvVars.Keys.CopyTo(keys, 0); + Logger.DebugFormat("Env vars of the process with PID={0}: {1}", new Object[] {pid, String.Join(",", keys)}); } return; } + else + { + // We just skip this check + affiliatedServiceId = null; + } // Check the service ID value - if (!ServiceId.Equals(affiliatedServiceId)) + if (CheckWinSWEnvironmentVariable && !ServiceId.Equals(affiliatedServiceId)) { Logger.Warn("The process " + pid + " has been started by Windows service with ID='" + affiliatedServiceId + "'. " + "It is another service (current service id is '" + ServiceId + "'), hence the process won't be terminated."); diff --git a/src/Test/winswTests/Extensions/ExtensionTestBase.cs b/src/Test/winswTests/Extensions/ExtensionTestBase.cs index c7e8541..bd1543e 100644 --- a/src/Test/winswTests/Extensions/ExtensionTestBase.cs +++ b/src/Test/winswTests/Extensions/ExtensionTestBase.cs @@ -15,7 +15,7 @@ namespace winswTests.Extensions /// /// Type of the extension /// String for Type locator, which includes class and assembly names - protected static String getExtensionClassNameWithAssembly(Type type) + public static String getExtensionClassNameWithAssembly(Type type) { return type.ToString() + ", " + type.Assembly; } diff --git a/src/Test/winswTests/Extensions/RunawayProcessKillerTest.cs b/src/Test/winswTests/Extensions/RunawayProcessKillerTest.cs index 0ea5022..31f950a 100644 --- a/src/Test/winswTests/Extensions/RunawayProcessKillerTest.cs +++ b/src/Test/winswTests/Extensions/RunawayProcessKillerTest.cs @@ -3,6 +3,12 @@ using NUnit.Framework; using winsw.Extensions; using winsw.Plugins.SharedDirectoryMapper; using winsw.Plugins.RunawayProcessKiller; +using winswTests.Util; +using System.IO; +using System.Diagnostics; +using winsw.Util; +using System; +using System.Collections.Generic; namespace winswTests.Extensions { @@ -58,5 +64,63 @@ namespace winswTests.Extensions manager.FireOnWrapperStarted(); manager.FireBeforeWrapperStopped(); } + + [Test] + public void ShouldKillTheSpawnedProcess() + { + var winswId = "myAppWithRunaway"; + var extensionId = "runawayProcessKiller"; + var tmpDir = FilesystemTestHelper.CreateTmpDirectory(); + + // Prepare the env var + String varName = WinSWSystem.ENVVAR_NAME_SERVICE_ID; + var env = new Dictionary(); + env.Add("varName", winswId); + + // Spawn the test process + var scriptFile = Path.Combine(tmpDir, "dosleep.bat"); + var envFile = Path.Combine(tmpDir, "env.txt"); + File.WriteAllText(scriptFile, "set > " + envFile + "\nsleep 100500"); + Process proc = new Process(); + var ps = proc.StartInfo; + ps.FileName = scriptFile; + ProcessHelper.StartProcessAndCallbackForExit(proc, envVars: env); + + try + { + // Generate extension and ensure that the roundtrip is correct + //TODO: checkWinSWEnvironmentVariable should be true, but it does not work due to proc.StartInfo.EnvironmentVariables + var pidfile = Path.Combine(tmpDir, "process.pid"); + var sd = ConfigXmlBuilder.create(id: winswId) + .WithRunawayProcessKiller(new RunawayProcessKillerExtension(pidfile, checkWinSWEnvironmentVariable: false), extensionId) + .ToServiceDescriptor(); + WinSWExtensionManager manager = new WinSWExtensionManager(sd); + manager.LoadExtensions(); + var extension = manager.Extensions[extensionId] as RunawayProcessKillerExtension; + Assert.IsNotNull(extension, "RunawayProcessKillerExtension should be loaded"); + Assert.AreEqual(pidfile, extension.Pidfile, "PidFile should have been retained during the config roundtrip"); + + // Inject PID + File.WriteAllText(pidfile, proc.Id.ToString()); + + // Try to terminate + Assert.That(!proc.HasExited, "Process " + proc + " has exited before the RunawayProcessKiller extension invocation"); + extension.OnWrapperStarted(); + Assert.That(proc.HasExited, "Process " + proc + " should have been terminated by RunawayProcessKiller"); + } + finally + { + if (!proc.HasExited) + { + Console.Error.WriteLine("Test: Killing runaway process with ID=" + proc.Id); + ProcessHelper.StopProcessAndChildren(proc.Id, TimeSpan.FromMilliseconds(100), false); + if (!proc.HasExited) + { + // The test is failed here anyway, but we add additional diagnostics info + Console.Error.WriteLine("Test: ProcessHelper failed to properly terminate process with ID=" + proc.Id); + } + } + } + } } } diff --git a/src/Test/winswTests/Util/ConfigXmlBuilder.cs b/src/Test/winswTests/Util/ConfigXmlBuilder.cs index 15c3818..9fc75f4 100644 --- a/src/Test/winswTests/Util/ConfigXmlBuilder.cs +++ b/src/Test/winswTests/Util/ConfigXmlBuilder.cs @@ -2,6 +2,8 @@ using System.Collections.Generic; using System.Text; using winsw; +using winsw.Plugins.RunawayProcessKiller; +using winswTests.Extensions; namespace winswTests.Util { @@ -16,6 +18,7 @@ namespace winswTests.Util public string Executable { get; set; } public bool PrintXMLVersion { get; set; } public string XMLComment { get; set; } + public List ExtensionXmls { get; private set; } private List configEntries; @@ -23,6 +26,7 @@ namespace winswTests.Util private ConfigXmlBuilder() { configEntries = new List(); + ExtensionXmls = new List(); } public static ConfigXmlBuilder create(string id = null, string name = null, @@ -63,6 +67,18 @@ namespace winswTests.Util // We do not care much about pretty formatting here str.AppendFormat(" {0}\n", entry); } + + // Extensions + if (ExtensionXmls.Count > 0) + { + str.Append(" \n"); + foreach (string xml in ExtensionXmls) + { + str.Append(xml); + } + str.Append(" \n"); + } + str.Append("\n"); string res = str.ToString(); if (dumpConfig) @@ -88,5 +104,20 @@ namespace winswTests.Util { return WithRawEntry(String.Format("<{0}>{1}", tagName, value)); } + + public ConfigXmlBuilder WithRunawayProcessKiller(RunawayProcessKillerExtension ext, string extensionId = "killRunawayProcess", bool enabled = true) + { + var fullyQualifiedExtensionName = ExtensionTestBase.getExtensionClassNameWithAssembly(typeof(RunawayProcessKillerExtension)); + StringBuilder str = new StringBuilder(); + str.AppendFormat(" \n", new Object[] { enabled, fullyQualifiedExtensionName, extensionId}); + str.AppendFormat(" {0}\n", ext.Pidfile); + str.AppendFormat(" {0}\n", ext.StopTimeout.TotalMilliseconds); + str.AppendFormat(" {0}\n", ext.StopParentProcessFirst); + str.AppendFormat(" {0}\n", ext.CheckWinSWEnvironmentVariable); + str.Append( " \n"); + ExtensionXmls.Add(str.ToString()); + + return this; + } } } diff --git a/src/Test/winswTests/Util/FilesystemTestHelper.cs b/src/Test/winswTests/Util/FilesystemTestHelper.cs new file mode 100644 index 0000000..00bf373 --- /dev/null +++ b/src/Test/winswTests/Util/FilesystemTestHelper.cs @@ -0,0 +1,46 @@ +using NUnit.Framework; +using System; +using System.Collections.Generic; +using System.IO; +using System.Text; + +namespace winswTests.Util +{ + class FilesystemTestHelper + { + /// + /// Creates a temporary directory for testing. + /// + /// tmp Dir + public static string CreateTmpDirectory(String testName = null) + { + string tempDirectory = Path.Combine(Path.GetTempPath(), "winswTests_" + (testName ?? "") + Path.GetRandomFileName()); + Directory.CreateDirectory(tempDirectory); + Console.Out.WriteLine("Created the temporary directory: {0}", tempDirectory); + return tempDirectory; + } + + /// + /// Parses output of the "set" command from the file + /// + /// File path + /// Dictionary of the strings. + public static Dictionary parseSetOutput(string filePath) + { + Dictionary res = new Dictionary(); + var lines = File.ReadAllLines(filePath); + foreach(var line in lines) { + var parsed = line.Split("=".ToCharArray(), 2); + if (parsed.Length == 2) + { + res.Add(parsed[0], parsed[1]); + } + else + { + Assert.Fail("Wrong line in the parsed Set output file: " + line); + } + } + return res; + } + } +} diff --git a/src/Test/winswTests/Util/ProcessHelperTest.cs b/src/Test/winswTests/Util/ProcessHelperTest.cs new file mode 100644 index 0000000..9bff216 --- /dev/null +++ b/src/Test/winswTests/Util/ProcessHelperTest.cs @@ -0,0 +1,51 @@ +using System; +using System.Diagnostics; +using NUnit.Framework; +using winsw; +using System.IO; +using winsw.Util; + +namespace winswTests.Util +{ + + [TestFixture] + class ProcessHelperTest + { + /// + /// Also reported as JENKINS-42744 + /// + [Test] + public void ShouldPropagateVariablesInUppercase() + { + var tmpDir = FilesystemTestHelper.CreateTmpDirectory(); + String envFile = Path.Combine(tmpDir, "env.properties"); + String scriptFile = Path.Combine(tmpDir, "printenv.bat"); + File.WriteAllText(scriptFile, "set > " + envFile); + + + Process proc = new Process(); + var ps = proc.StartInfo; + ps.FileName = scriptFile; + + ProcessHelper.StartProcessAndCallbackForExit(proc); + var exited = proc.WaitForExit(5000); + if (!exited) + { + Assert.Fail("Process " + proc + " didn't exit after 5 seconds"); + } + + // Check several veriables, which are expected to be in Uppercase + var envVars = FilesystemTestHelper.parseSetOutput(envFile); + String[] keys = new String[envVars.Count]; + envVars.Keys.CopyTo(keys, 0); + String availableVars = "[" + String.Join(",", keys) + "]"; + Assert.That(envVars.ContainsKey("PROCESSOR_ARCHITECTURE"), "No PROCESSOR_ARCHITECTURE in the injected vars: " + availableVars); + Assert.That(envVars.ContainsKey("COMPUTERNAME"), "No COMPUTERNAME in the injected vars: " + availableVars); + Assert.That(envVars.ContainsKey("PATHEXT"), "No PATHEXT in the injected vars: " + availableVars); + + // And just ensure that the parsing logic is case-sensitive + Assert.That(!envVars.ContainsKey("computername"), "Test error: the environment parsing logic is case-insensitive"); + + } + } +} diff --git a/src/Test/winswTests/winswTests.csproj b/src/Test/winswTests/winswTests.csproj index 7292731..27f3c39 100644 --- a/src/Test/winswTests/winswTests.csproj +++ b/src/Test/winswTests/winswTests.csproj @@ -60,6 +60,8 @@ + +