From 615519f6a327fea3c13490db022ff463c4025cef Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Fri, 31 Mar 2017 12:06:36 +0200 Subject: [PATCH 1/7] [JENKINS-42744] - Decouple the process start logic to a separate method in the helper class --- src/Core/ServiceWrapper/Main.cs | 74 ++++++------------------ src/Core/WinSWCore/Util/ProcessHelper.cs | 73 ++++++++++++++++++++++- 2 files changed, 91 insertions(+), 56 deletions(-) diff --git a/src/Core/ServiceWrapper/Main.cs b/src/Core/ServiceWrapper/Main.cs index be83d5e..b9e6a3e 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; + string msg = processToStart.Id + " - " + processToStart.StartInfo.FileName + " " + processToStart.StartInfo.Arguments; - foreach (string key in _envs.Keys) + // Define handler of the completed process + ProcessCompletionCallback processCompletionCallback = delegate(Process proc) { - 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 - { - 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..fd4fdc1 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,76 @@ namespace winsw.Util } } - // TODO: Also move StartProcess methods once LogEvent()/WriteEvent() mess gets solved + //TODO: generalize API + /// + /// 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 + public static void StartProcessAndCallbackForExit(Process processToStart, String executable, string arguments, Dictionary envVars, + string workingDirectory, ProcessPriorityClass priority, ProcessCompletionCallback callback) + { + var ps = processToStart.StartInfo; + ps.FileName = executable; + ps.Arguments = arguments; + ps.WorkingDirectory = 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 envVars.Keys) + { + Environment.SetEnvironmentVariable(key, envVars[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: move outside, stubbed to reproduce the issue + // 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()] = "myapp";// _descriptor.Id; + // Environment.SetEnvironmentVariable(WinSWSystem.ENVVAR_NAME_SERVICE_ID.ToLower(), _descriptor.Id); + + processToStart.Start(); + Logger.Info("Started process " + processToStart.Id); + + if (priority != ProcessPriorityClass.Normal) + processToStart.PriorityClass = priority; + + // monitor the completion of the process + 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); } From 9fc518a3d06613d5393bdd479f63046ebe4f3928 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Fri, 31 Mar 2017 13:02:53 +0200 Subject: [PATCH 2/7] [JENKINS-42744] - Reproduce the issue in the unit test --- src/Core/WinSWCore/Util/ProcessHelper.cs | 39 ++++++++------- .../winswTests/Util/FilesystemTestHelper.cs | 37 ++++++++++++++ src/Test/winswTests/Util/ProcessHelperTest.cs | 48 +++++++++++++++++++ src/Test/winswTests/winswTests.csproj | 2 + 4 files changed, 110 insertions(+), 16 deletions(-) create mode 100644 src/Test/winswTests/Util/FilesystemTestHelper.cs create mode 100644 src/Test/winswTests/Util/ProcessHelperTest.cs diff --git a/src/Core/WinSWCore/Util/ProcessHelper.cs b/src/Core/WinSWCore/Util/ProcessHelper.cs index fd4fdc1..3836afa 100644 --- a/src/Core/WinSWCore/Util/ProcessHelper.cs +++ b/src/Core/WinSWCore/Util/ProcessHelper.cs @@ -118,7 +118,6 @@ namespace winsw.Util } } - //TODO: generalize API /// /// Starts a process and asynchronosly waits for its termination. /// Once the process exits, the callback will be invoked. @@ -129,24 +128,27 @@ namespace winsw.Util /// Additional environment variables /// Working directory /// Priority - /// Completion callback - public static void StartProcessAndCallbackForExit(Process processToStart, String executable, string arguments, Dictionary envVars, - string workingDirectory, ProcessPriorityClass priority, ProcessCompletionCallback callback) + /// 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.Arguments = arguments; - ps.WorkingDirectory = workingDirectory; + 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; - foreach (string key in envVars.Keys) + if (envVars != null) { - Environment.SetEnvironmentVariable(key, envVars[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) + foreach (string key in envVars.Keys) + { + Environment.SetEnvironmentVariable(key, envVars[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: move outside, stubbed to reproduce the issue @@ -157,15 +159,20 @@ namespace winsw.Util processToStart.Start(); Logger.Info("Started process " + processToStart.Id); - if (priority != ProcessPriorityClass.Normal) - processToStart.PriorityClass = priority; + if (priority != null && priority.Value != ProcessPriorityClass.Normal) + { + processToStart.PriorityClass = priority.Value; + } // monitor the completion of the process - StartThread(delegate + if (callback != null) { - processToStart.WaitForExit(); - callback(processToStart); - }); + StartThread(delegate + { + processToStart.WaitForExit(); + callback(processToStart); + }); + } } /// diff --git a/src/Test/winswTests/Util/FilesystemTestHelper.cs b/src/Test/winswTests/Util/FilesystemTestHelper.cs new file mode 100644 index 0000000..63c1fb3 --- /dev/null +++ b/src/Test/winswTests/Util/FilesystemTestHelper.cs @@ -0,0 +1,37 @@ +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) { + line.Split("=".ToCharArray(), 2); + } + return res; + } + } +} diff --git a/src/Test/winswTests/Util/ProcessHelperTest.cs b/src/Test/winswTests/Util/ProcessHelperTest.cs new file mode 100644 index 0000000..d537e26 --- /dev/null +++ b/src/Test/winswTests/Util/ProcessHelperTest.cs @@ -0,0 +1,48 @@ +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); + Assert.That(envVars.ContainsKey("PROCESSOR_ARCHITECTURE"), "No PROCESSOR_ARCHITECTURE in the injected vars"); + Assert.That(envVars.ContainsKey("COMPUTERNAME"), "No COMPUTERNAME in the injected vars"); + Assert.That(envVars.ContainsKey("PATHEXT"), "No PATHEXT in the injected vars"); + + // 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 @@ + + From bca9bafc66515e3d69ec5f61701252f4d271ef37 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Fri, 31 Mar 2017 15:10:57 +0200 Subject: [PATCH 3/7] [JENKINS-42744] - Improve the ProcessHelperTes, add RunawayProcessKillerTest for the affected logic --- .../RunawayProcessKillerExtension.cs | 14 +++-- .../Extensions/RunawayProcessKillerTest.cs | 61 +++++++++++++++++++ src/Test/winswTests/Util/ConfigXmlBuilder.cs | 30 +++++++++ .../winswTests/Util/FilesystemTestHelper.cs | 13 +++- src/Test/winswTests/Util/ProcessHelperTest.cs | 9 ++- 5 files changed, 117 insertions(+), 10 deletions(-) diff --git a/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs b/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs index 0677549..562b17a 100644 --- a/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs +++ b/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs @@ -38,9 +38,11 @@ namespace winsw.Plugins.RunawayProcessKiller // Default initializer } - public RunawayProcessKillerExtension(String pidfile) + public RunawayProcessKillerExtension(String pidfile, int stopTimeoutMs = 5000, bool stopParentFirst = false) { this.Pidfile = pidfile; + this.StopTimeout = TimeSpan.FromMilliseconds(5000); + this.StopParentProcessFirst = stopParentFirst; } public override void Configure(ServiceDescriptor descriptor, XmlNode node) @@ -89,6 +91,7 @@ namespace winsw.Plugins.RunawayProcessKiller } // Now check the process + Logger.DebugFormat("Checking the potentially runaway process with PID={0}", pid); Process proc; try { @@ -113,11 +116,12 @@ namespace winsw.Plugins.RunawayProcessKiller else { 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; } diff --git a/src/Test/winswTests/Extensions/RunawayProcessKillerTest.cs b/src/Test/winswTests/Extensions/RunawayProcessKillerTest.cs index 0ea5022..efa56cc 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,60 @@ 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 + var pidfile = Path.Combine(tmpDir, "process.pid"); + var sd = ConfigXmlBuilder.create(id: winswId).WithRunawayProcessKiller(new RunawayProcessKillerExtension(pidfile), 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..9416a29 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,19 @@ 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.Append( " \n"); + ExtensionXmls.Add(str.ToString()); + + return this; + } } } diff --git a/src/Test/winswTests/Util/FilesystemTestHelper.cs b/src/Test/winswTests/Util/FilesystemTestHelper.cs index 63c1fb3..00bf373 100644 --- a/src/Test/winswTests/Util/FilesystemTestHelper.cs +++ b/src/Test/winswTests/Util/FilesystemTestHelper.cs @@ -1,4 +1,5 @@ -using System; +using NUnit.Framework; +using System; using System.Collections.Generic; using System.IO; using System.Text; @@ -29,7 +30,15 @@ namespace winswTests.Util Dictionary res = new Dictionary(); var lines = File.ReadAllLines(filePath); foreach(var line in lines) { - line.Split("=".ToCharArray(), 2); + 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 index d537e26..9bff216 100644 --- a/src/Test/winswTests/Util/ProcessHelperTest.cs +++ b/src/Test/winswTests/Util/ProcessHelperTest.cs @@ -36,9 +36,12 @@ namespace winswTests.Util // Check several veriables, which are expected to be in Uppercase var envVars = FilesystemTestHelper.parseSetOutput(envFile); - Assert.That(envVars.ContainsKey("PROCESSOR_ARCHITECTURE"), "No PROCESSOR_ARCHITECTURE in the injected vars"); - Assert.That(envVars.ContainsKey("COMPUTERNAME"), "No COMPUTERNAME in the injected vars"); - Assert.That(envVars.ContainsKey("PATHEXT"), "No PATHEXT in the injected vars"); + 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"); From 9cfdcf4ae7256f168689158f0078ad176bc83fb2 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Fri, 31 Mar 2017 15:28:02 +0200 Subject: [PATCH 4/7] [JENKINS-42744] - Allow ignoring the WINSW_SERVICE_ID env variable (test-only for now) --- .../RunawayProcessKillerExtension.cs | 21 ++++++++++++++++--- .../Extensions/RunawayProcessKillerTest.cs | 5 ++++- src/Test/winswTests/Util/ConfigXmlBuilder.cs | 1 + 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs b/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs index 562b17a..94abf59 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,11 +44,12 @@ namespace winsw.Plugins.RunawayProcessKiller // Default initializer } - public RunawayProcessKillerExtension(String pidfile, int stopTimeoutMs = 5000, bool stopParentFirst = false) + 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) @@ -53,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; } /// @@ -113,7 +123,7 @@ namespace winsw.Plugins.RunawayProcessKiller { 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 WinSW, hence it won't be terminated."); @@ -125,9 +135,14 @@ namespace winsw.Plugins.RunawayProcessKiller } 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/RunawayProcessKillerTest.cs b/src/Test/winswTests/Extensions/RunawayProcessKillerTest.cs index efa56cc..31f950a 100644 --- a/src/Test/winswTests/Extensions/RunawayProcessKillerTest.cs +++ b/src/Test/winswTests/Extensions/RunawayProcessKillerTest.cs @@ -89,8 +89,11 @@ namespace winswTests.Extensions 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), extensionId).ToServiceDescriptor(); + 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; diff --git a/src/Test/winswTests/Util/ConfigXmlBuilder.cs b/src/Test/winswTests/Util/ConfigXmlBuilder.cs index 9416a29..9fc75f4 100644 --- a/src/Test/winswTests/Util/ConfigXmlBuilder.cs +++ b/src/Test/winswTests/Util/ConfigXmlBuilder.cs @@ -113,6 +113,7 @@ namespace winswTests.Util 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()); From f81f5d3c574d6b72f08a9325845c25541e6c34ab Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Fri, 31 Mar 2017 15:36:56 +0200 Subject: [PATCH 5/7] [FIXED JENKINS-42744] - Do not inject ps.EnvironmentVariables explicitly --- src/Core/WinSWCore/Util/ProcessHelper.cs | 8 ++------ .../RunawayProcessKiller/RunawayProcessKillerExtension.cs | 3 ++- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Core/WinSWCore/Util/ProcessHelper.cs b/src/Core/WinSWCore/Util/ProcessHelper.cs index 3836afa..03327aa 100644 --- a/src/Core/WinSWCore/Util/ProcessHelper.cs +++ b/src/Core/WinSWCore/Util/ProcessHelper.cs @@ -147,15 +147,11 @@ namespace winsw.Util foreach (string key in envVars.Keys) { Environment.SetEnvironmentVariable(key, envVars[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) + // 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) } } - //TODO: move outside, stubbed to reproduce the issue - // 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()] = "myapp";// _descriptor.Id; - // Environment.SetEnvironmentVariable(WinSWSystem.ENVVAR_NAME_SERVICE_ID.ToLower(), _descriptor.Id); - processToStart.Start(); Logger.Info("Started process " + processToStart.Id); diff --git a/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs b/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs index 94abf59..66d7e0f 100644 --- a/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs +++ b/src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs @@ -118,9 +118,10 @@ 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 if (CheckWinSWEnvironmentVariable) From 8cd58531f6d3bd3cfedc16668d9e3cfb765bae19 Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Fri, 31 Mar 2017 16:01:01 +0200 Subject: [PATCH 6/7] [JENKINS-42744] - Just another proof that some testing coverage is required for the service management logic --- src/Core/ServiceWrapper/Main.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Core/ServiceWrapper/Main.cs b/src/Core/ServiceWrapper/Main.cs index b9e6a3e..c6ce65f 100644 --- a/src/Core/ServiceWrapper/Main.cs +++ b/src/Core/ServiceWrapper/Main.cs @@ -400,11 +400,11 @@ namespace winsw private void StartProcess(Process processToStart, string arguments, String executable) { - string msg = processToStart.Id + " - " + processToStart.StartInfo.FileName + " " + processToStart.StartInfo.Arguments; - + // Define handler of the completed process ProcessCompletionCallback processCompletionCallback = delegate(Process proc) { + string msg = processToStart.Id + " - " + processToStart.StartInfo.FileName + " " + processToStart.StartInfo.Arguments; try { if (_orderlyShutdown) From 5637c406aa7411c5764cc19a6318102b522fa1af Mon Sep 17 00:00:00 2001 From: Oleg Nenashev Date: Fri, 31 Mar 2017 17:01:42 +0200 Subject: [PATCH 7/7] ExtensionTestBase should provide public methods within the test class --- src/Test/winswTests/Extensions/ExtensionTestBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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; }