From b6ba7332e56ef7c9d808f5e4e103c36d4d9b3953 Mon Sep 17 00:00:00 2001
From: Jordan Brown <Cyberboss@users.noreply.github.com>
Date: Mon, 14 Jun 2021 22:30:19 -0400
Subject: [PATCH] WindowsProcessFeatures cleanup

- Adds logging around Open/Suspend/ResumeThread calls.
- Call Process.Refresh before accessing Process.Threads, potentially fixing issues with a stale thead ID cache.
- Potentially fixes #1279
---
 .../System/WindowsProcessFeatures.cs          | 32 +++++++++++++++++--
 .../System/TestProcessFeatures.cs             |  2 +-
 .../Instance/WatchdogTest.cs                  |  2 +-
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/src/Tgstation.Server.Host/System/WindowsProcessFeatures.cs b/src/Tgstation.Server.Host/System/WindowsProcessFeatures.cs
index d8c66615f2..81fd20217f 100644
--- a/src/Tgstation.Server.Host/System/WindowsProcessFeatures.cs
+++ b/src/Tgstation.Server.Host/System/WindowsProcessFeatures.cs
@@ -7,6 +7,7 @@ using System.Threading;
 using System.Threading.Tasks;
 
 using BetterWin32Errors;
+using Microsoft.Extensions.Logging;
 
 using Tgstation.Server.Api.Models;
 using Tgstation.Server.Host.IO;
@@ -17,17 +18,37 @@ namespace Tgstation.Server.Host.System
 	/// <inheritdoc />
 	sealed class WindowsProcessFeatures : IProcessFeatures
 	{
+		/// <summary>
+		/// The <see cref="ILogger"/> for the <see cref="WindowsProcessFeatures"/>.
+		/// </summary>
+		readonly ILogger<WindowsProcessFeatures> logger;
+
+		/// <summary>
+		/// Initializes a new instance of the <see cref="WindowsProcessFeatures"/> class.
+		/// </summary>
+		/// <param name="logger">The value of logger.</param>
+		public WindowsProcessFeatures(ILogger<WindowsProcessFeatures> logger)
+		{
+			this.logger = logger ?? throw new ArgumentNullException(nameof(logger));
+		}
+
 		/// <inheritdoc />
 		public void ResumeProcess(global::System.Diagnostics.Process process)
 		{
 			if (process == null)
 				throw new ArgumentNullException(nameof(process));
 
+			process.Refresh();
 			foreach (ProcessThread thread in process.Threads)
 			{
-				var pOpenThread = NativeMethods.OpenThread(NativeMethods.ThreadAccess.SuspendResume, false, (uint)thread.Id);
+				var threadId = (uint)thread.Id;
+				logger.LogTrace("Suspending thread {0}...", threadId);
+				var pOpenThread = NativeMethods.OpenThread(NativeMethods.ThreadAccess.SuspendResume, false, threadId);
 				if (pOpenThread == IntPtr.Zero)
+				{
+					logger.LogDebug(new Win32Exception(), "Failed to open thread {0}!", threadId);
 					continue;
+				}
 
 				try
 				{
@@ -47,11 +68,18 @@ namespace Tgstation.Server.Host.System
 			if (process == null)
 				throw new ArgumentNullException(nameof(process));
 
+			process.Refresh();
 			foreach (ProcessThread thread in process.Threads)
 			{
-				var pOpenThread = NativeMethods.OpenThread(NativeMethods.ThreadAccess.SuspendResume, false, (uint)thread.Id);
+				var threadId = (uint)thread.Id;
+				logger.LogTrace("Resuming thread {0}...", threadId);
+				var pOpenThread = NativeMethods.OpenThread(NativeMethods.ThreadAccess.SuspendResume, false, threadId);
 				if (pOpenThread == IntPtr.Zero)
+				{
+					logger.LogDebug(new Win32Exception(), "Failed to open thread {0}!", threadId);
 					continue;
+				}
+
 				try
 				{
 					if (NativeMethods.SuspendThread(pOpenThread) == UInt32.MaxValue)
diff --git a/tests/Tgstation.Server.Host.Tests/System/TestProcessFeatures.cs b/tests/Tgstation.Server.Host.Tests/System/TestProcessFeatures.cs
index 93d5acde14..d3ac6aaab0 100644
--- a/tests/Tgstation.Server.Host.Tests/System/TestProcessFeatures.cs
+++ b/tests/Tgstation.Server.Host.Tests/System/TestProcessFeatures.cs
@@ -21,7 +21,7 @@ namespace Tgstation.Server.Host.System.Tests
 		public void Init()
 		{
 			features = new PlatformIdentifier().IsWindows
-				? (IProcessFeatures)new WindowsProcessFeatures()
+				? (IProcessFeatures)new WindowsProcessFeatures(Mock.Of<ILogger<WindowsProcessFeatures>>())
 				: new PosixProcessFeatures(new Lazy<IProcessExecutor>(() => null), new DefaultIOManager(), Mock.Of<ILogger<PosixProcessFeatures>>());
 		}
 
diff --git a/tests/Tgstation.Server.Tests/Instance/WatchdogTest.cs b/tests/Tgstation.Server.Tests/Instance/WatchdogTest.cs
index 3159dd1be4..41acb6b011 100644
--- a/tests/Tgstation.Server.Tests/Instance/WatchdogTest.cs
+++ b/tests/Tgstation.Server.Tests/Instance/WatchdogTest.cs
@@ -227,7 +227,7 @@ namespace Tgstation.Server.Tests.Instance
 			IProcessExecutor executor = null;
 			executor = new ProcessExecutor(
 				new PlatformIdentifier().IsWindows
-					? (IProcessFeatures)new WindowsProcessFeatures()
+					? (IProcessFeatures)new WindowsProcessFeatures(Mock.Of<ILogger<WindowsProcessFeatures>>())
 					: new PosixProcessFeatures(new Lazy<IProcessExecutor>(() => executor), Mock.Of<IIOManager>(), Mock.Of<ILogger<PosixProcessFeatures>>()),
 				Mock.Of<ILogger<ProcessExecutor>>(),
 				LoggerFactory.Create(x => { }));