From 03f53b4cc1f1a7a97829e6bc0d43eed4588cd32f Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Mon, 22 May 2017 12:36:07 -0700 Subject: [PATCH] Respond to PR feedback --- build/scripts/cibuild.ps1 | 7 ++++ .../RunTests/Cache/CachingTestExecutor.cs | 2 +- .../Source/RunTests/Cache/LocalDataStorage.cs | 6 ++-- src/Tools/Source/RunTests/Logger.cs | 19 +++++++++++ src/Tools/Source/RunTests/ProcessRunner.cs | 2 +- src/Tools/Source/RunTests/ProcessUtil.cs | 20 ----------- src/Tools/Source/RunTests/Program.cs | 33 ++++++++----------- 7 files changed, 44 insertions(+), 45 deletions(-) diff --git a/build/scripts/cibuild.ps1 b/build/scripts/cibuild.ps1 index f5eca9fe884..6a7a450548e 100644 --- a/build/scripts/cibuild.ps1 +++ b/build/scripts/cibuild.ps1 @@ -55,6 +55,13 @@ function Terminate-BuildProcesses() { # Ensure that procdump is available on the machine. Returns the path to the directory that contains # the procdump binaries (both 32 and 64 bit) function Ensure-ProcDump() { + + # Jenkins images default to having procdump installed in the root. Use that if available to avoid + # an unnecessary download. + if (Test-Path "c:\SysInternals\procdump.exe") { + return "c:\SysInternals"; + } + $toolsDir = Join-Path $binariesDir "Tools" $outDir = Join-Path $toolsDir "ProcDump" $filePath = Join-Path $outDir "procdump.exe" diff --git a/src/Tools/Source/RunTests/Cache/CachingTestExecutor.cs b/src/Tools/Source/RunTests/Cache/CachingTestExecutor.cs index 9d61b91bb8b..c0ff6912ea3 100644 --- a/src/Tools/Source/RunTests/Cache/CachingTestExecutor.cs +++ b/src/Tools/Source/RunTests/Cache/CachingTestExecutor.cs @@ -124,7 +124,7 @@ private async Task CacheTestResult(ContentFile contentFile, TestResult testResul } catch (Exception ex) { - Logger.Log($"Failed to create cached {ex}"); + Logger.Log("Failed to create cached", ex); } } } diff --git a/src/Tools/Source/RunTests/Cache/LocalDataStorage.cs b/src/Tools/Source/RunTests/Cache/LocalDataStorage.cs index 0e75a8daa5a..e862b24cca9 100644 --- a/src/Tools/Source/RunTests/Cache/LocalDataStorage.cs +++ b/src/Tools/Source/RunTests/Cache/LocalDataStorage.cs @@ -80,7 +80,7 @@ public bool TryGetCachedTestResult(string checksum, out CachedTestResult testRes catch (Exception e) { // Okay for exception to occur here on I/O - Logger.Log($"Failed to read cache {checksum} {e.Message}"); + Logger.Log($"Failed to read cache {checksum}", e); } return false; @@ -107,7 +107,7 @@ public Task AddCachedTestResult(AssemblyInfo assemblyInfo, ContentFile contentFi catch (Exception e) { // I/O errors are expected and okay here. - Logger.Log($"Failed to log {checksum} {e.Message}"); + Logger.Log($"Failed to log {checksum}", e); FileUtil.DeleteDirectory(storagePath); } @@ -159,7 +159,7 @@ private void CleanupStorage() } catch (Exception ex) { - Logger.Log($"Unable to cleanup storage {ex.Message}"); + Logger.Log("Unable to cleanup storage", ex); } } } diff --git a/src/Tools/Source/RunTests/Logger.cs b/src/Tools/Source/RunTests/Logger.cs index 8f746f5a6fc..19a91828de3 100644 --- a/src/Tools/Source/RunTests/Logger.cs +++ b/src/Tools/Source/RunTests/Logger.cs @@ -26,6 +26,25 @@ internal static void LogError(Exception ex, string line) } } + internal static void Log(string message, Exception ex) + { + lock (s_lines) + { + s_lines.Add(message); + s_lines.Add(ex.Message); + s_lines.Add(ex.StackTrace); + } + } + + internal static void Log(Exception ex) + { + lock (s_lines) + { + s_lines.Add(ex.Message); + s_lines.Add(ex.StackTrace); + } + } + internal static void Log(string line) { lock (s_lines) diff --git a/src/Tools/Source/RunTests/ProcessRunner.cs b/src/Tools/Source/RunTests/ProcessRunner.cs index e012440eb1f..2e8d0047a6f 100644 --- a/src/Tools/Source/RunTests/ProcessRunner.cs +++ b/src/Tools/Source/RunTests/ProcessRunner.cs @@ -36,8 +36,8 @@ public static void OpenFile(string file) public static Task RunProcessAsync( string executable, string arguments, - bool lowPriority, CancellationToken cancellationToken, + bool lowPriority = false, string workingDirectory = null, bool captureOutput = false, bool displayWindow = true, diff --git a/src/Tools/Source/RunTests/ProcessUtil.cs b/src/Tools/Source/RunTests/ProcessUtil.cs index bb8a0b594da..abb474ba92f 100644 --- a/src/Tools/Source/RunTests/ProcessUtil.cs +++ b/src/Tools/Source/RunTests/ProcessUtil.cs @@ -78,25 +78,5 @@ internal static List GetProcessTree(Process process) return list; } - - internal static bool Is64Bit(Process process) - { - if (Environment.GetEnvironmentVariable("PROCESSOR_ARCHITECTURE") == "x86") - { - return false; - } - - bool isWow64; - if (!IsWow64Process(process.Handle, out isWow64)) - { - throw new Exception($"{nameof(IsWow64Process)} failed with {Marshal.GetLastWin32Error()}"); - } - - return !isWow64; - } - - [DllImport("kernel32.dll", SetLastError = true, CallingConvention = CallingConvention.Winapi)] - [return: MarshalAs(UnmanagedType.Bool)] - private static extern bool IsWow64Process([In] IntPtr process, [Out] out bool wow64Process); } } diff --git a/src/Tools/Source/RunTests/Program.cs b/src/Tools/Source/RunTests/Program.cs index 0665ca07895..069e3491896 100644 --- a/src/Tools/Source/RunTests/Program.cs +++ b/src/Tools/Source/RunTests/Program.cs @@ -64,7 +64,7 @@ private static async Task Run(Options options, CancellationToken cancellati var finishedTask = await Task.WhenAny(timeoutTask, runTask); if (finishedTask == timeoutTask) { - HandleTimeout(options); + await HandleTimeout(options, cancellationToken); cts.Cancel(); try @@ -150,26 +150,18 @@ private static void WriteLogFile(Options options) /// Invoked when a timeout occurs and we need to dump all of the test processes and shut down /// the runnner. /// - private static void HandleTimeout(Options options) + private static async Task HandleTimeout(Options options, CancellationToken cancellationToken) { - string GetProcDumpFilePath(Process proc) => ProcessUtil.Is64Bit(proc) - ? Path.Combine(options.ProcDumpPath, "procdump64.exe") - : Path.Combine(options.ProcDumpPath, "procdump.exe"); + var procDumpFilePath = Path.Combine(options.ProcDumpPath, "procdump.exe"); - void DumpProcess(Process targetProcess, string dumpFilePath) + async Task DumpProcess(Process targetProcess, string dumpFilePath) { Console.Write($"Dumping {targetProcess.ProcessName} {targetProcess.Id} to {dumpFilePath} ... "); try { - var processStartInfo = new ProcessStartInfo(); - processStartInfo.FileName = GetProcDumpFilePath(targetProcess); - processStartInfo.Arguments = $"-accepteula -ma {targetProcess.Id} {dumpFilePath}"; - processStartInfo.CreateNoWindow = true; - processStartInfo.UseShellExecute = false; - processStartInfo.WindowStyle = ProcessWindowStyle.Hidden; - processStartInfo.RedirectStandardOutput = true; - var process = Process.Start(processStartInfo); - process.WaitForExit(); + var args = $"-accepteula -ma {targetProcess.Id} {dumpFilePath}"; + var processTask = ProcessRunner.RunProcessAsync(procDumpFilePath, args, cancellationToken); + var processOutput = await processTask; // The exit code for procdump doesn't obey standard windows rules. It will return non-zero // for succesful cases (possibly returning the count of dumps that were written). Best @@ -180,15 +172,16 @@ void DumpProcess(Process targetProcess, string dumpFilePath) } else { - Console.WriteLine($"FAILED with {process.ExitCode}"); - Console.WriteLine($"{processStartInfo.FileName} {processStartInfo.Arguments}"); - Console.WriteLine(process.StandardOutput.ReadToEnd()); + Console.WriteLine($"FAILED with {processOutput.ExitCode}"); + Console.WriteLine($"{procDumpFilePath} {args}"); + Console.WriteLine(string.Join(Environment.NewLine, processOutput.OutputLines)); } } - catch (Exception ex) + catch (Exception ex) when (!cancellationToken.IsCancellationRequested) { Console.WriteLine("FAILED"); Console.WriteLine(ex.Message); + Logger.Log("Failed to dump process", ex); } } @@ -204,7 +197,7 @@ void DumpProcess(Process targetProcess, string dumpFilePath) foreach (var proc in ProcessUtil.GetProcessTree(Process.GetCurrentProcess()).OrderBy(x => x.ProcessName)) { var dumpFilePath = Path.Combine(dumpDir, $"{proc.ProcessName}-{counter}.dmp"); - DumpProcess(proc, dumpFilePath); + await DumpProcess(proc, dumpFilePath); counter++; } } -- GitLab