From 0b6f27757257ecb49bd1b4db691524978a5c29d6 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Mon, 20 Jul 2026 11:49:53 +1000 Subject: [PATCH 1/2] Serialize WINWORD identification across concurrent diffword instances Concurrent diffword launches (eg Verify reporting several failed snapshots from one test run) snapshot overlapping PID sets and can claim the same, or each other's, WINWORD. A misidentified WINWORD is assigned to the wrong Job Object (or none), so killing its diffword (eg DiffEngineTray accept) leaves it running as an invisible zombie holding locks on the compared files. - Extract the snapshot-create-identify-assign sequence into CreateWordInJob, serialized across processes with a temp file lock (same pattern as diffexcel's SpreadsheetCompare) - Quit/kill the just-created instance when identification or assignment fails instead of leaking it - Log the assigned WINWORD pid and exit --- src/MsWordDiff.Tests/IdentifyLockTests.cs | 19 +++++ src/MsWordDiff/Word.cs | 91 +++++++++++++++++------ 2 files changed, 86 insertions(+), 24 deletions(-) create mode 100644 src/MsWordDiff.Tests/IdentifyLockTests.cs diff --git a/src/MsWordDiff.Tests/IdentifyLockTests.cs b/src/MsWordDiff.Tests/IdentifyLockTests.cs new file mode 100644 index 0000000..f158378 --- /dev/null +++ b/src/MsWordDiff.Tests/IdentifyLockTests.cs @@ -0,0 +1,19 @@ +[NotInParallel("IdentifyLock")] +public class IdentifyLockTests +{ + [Test] + public async Task SerializesConcurrentAcquires() + { + using var first = await Word.AcquireIdentifyLock(); + + var secondTask = Word.AcquireIdentifyLock(); + await Task.Delay(300); + await Assert.That(secondTask.IsCompleted).IsFalse() + .Because("The second acquire should block while the first holds the lock"); + + first.Dispose(); + + using var second = await secondTask; + await Assert.That(second).IsNotNull(); + } +} diff --git a/src/MsWordDiff/Word.cs b/src/MsWordDiff/Word.cs index 7a7c7cf..425c180 100644 --- a/src/MsWordDiff/Word.cs +++ b/src/MsWordDiff/Word.cs @@ -10,19 +10,8 @@ public static async Task Launch(string path1, string path2, bool quiet = false) var job = JobObject.Create(); - // Snapshot existing Word PIDs before creating the COM instance, then poll - // for the newly-spawned WINWORD.EXE so we can put it in the Job Object before - // any operation that can throw. This is critical for the DiffEngineTray - // "accept" flow: tray hard-kills diffword.exe (Process.Kill), which only - // reaps WINWORD via KILL_ON_JOB_CLOSE if WINWORD was actually assigned to - // our job. A short retry loop is needed because Process.GetProcessesByName - // can lag the Activator return — particularly under Click-to-Run / AppV - // where Word spawns through a launcher chain. - var existingPids = GetWordProcessIds(); - dynamic word = Activator.CreateInstance(wordType)!; - var process = WaitForNewWordProcess(existingPids, TimeSpan.FromSeconds(5)) - ?? throw new("Failed to locate the WINWORD.EXE process spawned by COM activation"); - JobObject.AssignProcess(job, process.Handle); + var (word, process) = await CreateWordInJob(wordType, job); + Log.Information("WINWORD {WordPid} assigned to job. Comparing {Path1} and {Path2}", process.Id, path1, path2); try { @@ -50,6 +39,7 @@ public static async Task Launch(string path1, string path2, bool quiet = false) SetForegroundWindow((IntPtr)word.ActiveWindow.Hwnd); await process.WaitForExitAsync(); + Log.Information("WINWORD {WordPid} exited", process.Id); Marshal.ReleaseComObject(compare); } @@ -67,7 +57,68 @@ public static async Task Launch(string path1, string path2, bool quiet = false) JobObject.Close(job); } - RestoreRibbon(wordType); + await RestoreRibbon(wordType); + } + + // Serialize the snapshot-launch-identify sequence across concurrent diffword + // instances (same pattern as diffexcel's SpreadsheetCompare). Without this, + // instances launched near-simultaneously (eg Verify reporting several failed + // snapshots from one test run) snapshot overlapping PID sets and can claim the + // same, or each other's, WINWORD. A misidentified WINWORD is assigned to the + // wrong Job Object (or none), so killing its diffword (DiffEngineTray "accept") + // leaves it running as an invisible zombie that still holds locks on the + // compared files. A file lock is used instead of a Mutex because file locks + // are not thread-affine, allowing async code within the critical section. + static readonly string identifyLockPath = Path.Combine(Path.GetTempPath(), "MsWordDiff.identify.lock"); + + internal static async Task AcquireIdentifyLock() + { + for (var i = 0; i < 300; i++) + { + try + { + return new(identifyLockPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None); + } + catch (IOException) + { + await Task.Delay(100); + } + } + + throw new IOException($"Failed to acquire lock file: {identifyLockPath}"); + } + + // Creates the Word COM instance and assigns its WINWORD to the job before any + // other operation. Snapshot existing Word PIDs before creating the COM instance, + // then poll for the newly-spawned WINWORD.EXE. This is critical for the + // DiffEngineTray "accept" flow: tray hard-kills diffword.exe (Process.Kill), + // which only reaps WINWORD via KILL_ON_JOB_CLOSE if WINWORD was actually + // assigned to our job. The poll retry loop is needed because + // Process.GetProcessesByName can lag the Activator return — particularly under + // Click-to-Run / AppV where Word spawns through a launcher chain. + // If identification or assignment fails, the just-created instance is quit or + // killed rather than left orphaned. + static async Task<(dynamic word, Process process)> CreateWordInJob(Type wordType, IntPtr job) + { + using (await AcquireIdentifyLock()) + { + var existingPids = GetWordProcessIds(); + dynamic word = Activator.CreateInstance(wordType)!; + Process? process = null; + try + { + process = WaitForNewWordProcess(existingPids, TimeSpan.FromSeconds(5)) + ?? throw new("Failed to locate the WINWORD.EXE process spawned by COM activation"); + JobObject.AssignProcess(job, process.Handle); + return (word, process); + } + catch + { + QuitAndKill(word, process); + Marshal.ReleaseComObject(word); + throw; + } + } } internal static dynamic LaunchCompare(dynamic word, dynamic doc1, dynamic doc2) @@ -170,18 +221,10 @@ static void MinimizeRibbon(dynamic word) // ribbon so the user's next normal Word session isn't affected. This instance // is assigned to its own Job Object and has a kill fallback to prevent zombies // (previously it had neither, making it the primary source of leaked processes). - static void RestoreRibbon(Type wordType) + static async Task RestoreRibbon(Type wordType) { var job = JobObject.Create(); - // Same poll-and-assign-before-throwables pattern as Launch. Previously this - // path had no fallback when FindNewWordProcess returned null, so a failed - // Quit() left the process orphaned (process was null → QuitAndKill couldn't - // force-kill, and the job had nothing assigned to reap). - var existingPids = GetWordProcessIds(); - dynamic word = Activator.CreateInstance(wordType)!; - var process = WaitForNewWordProcess(existingPids, TimeSpan.FromSeconds(5)) - ?? throw new("Failed to locate the WINWORD.EXE process spawned by COM activation"); - JobObject.AssignProcess(job, process.Handle); + var (word, process) = await CreateWordInJob(wordType, job); try { From fc9ffbf5f09e93c40db080c8dd150fe2160e3977 Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Mon, 20 Jul 2026 12:48:15 +1000 Subject: [PATCH 2/2] Update Directory.Build.props --- src/Directory.Build.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 62729ba..ad69909 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -1,7 +1,7 @@ - 1.0.1 + 1.0.2 Word, Diff, Compare A .NET tool that compares two Word/Excel documents using the Microsoft Word/Excel built-in document comparison features. enable