diff --git a/src/DiffEngine/DiffRunner.cs b/src/DiffEngine/DiffRunner.cs index 888c8bca..c21fabac 100644 --- a/src/DiffEngine/DiffRunner.cs +++ b/src/DiffEngine/DiffRunner.cs @@ -156,11 +156,12 @@ static LaunchResult InnerLaunch(TryResolveTool tryResolveTool, string tempFile, tool.CommandAndArguments(tempFile, targetFile, out var arguments, out var command); + var canKill = !tool.IsMdi; if (ProcessCleanup.TryGetProcessInfo(command, out var processCommand)) { if (tool.AutoRefresh) { - DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, tool.IsMdi, processCommand.Process); + DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, canKill, processCommand.Process); return LaunchResult.AlreadyRunningAndSupportsRefresh; } @@ -169,13 +170,13 @@ static LaunchResult InnerLaunch(TryResolveTool tryResolveTool, string tempFile, if (MaxInstance.Reached()) { - DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, tool.IsMdi, null); + DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, canKill, null); return LaunchResult.TooManyRunningDiffTools; } var processId = LaunchProcess(tool, arguments); - DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, !tool.IsMdi, processId); + DiffEngineTray.AddMove(tempFile, targetFile, tool.ExePath, arguments, canKill, processId); return LaunchResult.StartedNewInstance; } diff --git a/src/DiffEngine/Tray/DiffEngineTray.cs b/src/DiffEngine/Tray/DiffEngineTray.cs index a0245a0b..f582d78e 100644 --- a/src/DiffEngine/Tray/DiffEngineTray.cs +++ b/src/DiffEngine/Tray/DiffEngineTray.cs @@ -19,7 +19,7 @@ static DiffEngineTray() } } - public static bool IsRunning { get; } + public static bool IsRunning { get; internal set; } public static void AddDelete(string file) { diff --git a/src/DiffEngine/Tray/PiperClient.cs b/src/DiffEngine/Tray/PiperClient.cs index 75dc167a..62d8c616 100644 --- a/src/DiffEngine/Tray/PiperClient.cs +++ b/src/DiffEngine/Tray/PiperClient.cs @@ -10,7 +10,7 @@ public static Task SendDeleteAsync( Cancel cancel = default) { var payload = BuildDeletePayload(file); - return SendAsync(payload); + return SendAsync(payload, cancel); } static string BuildDeletePayload(string file) => @@ -41,7 +41,7 @@ public static Task SendMoveAsync( Cancel cancel = default) { var payload = BuildMovePayload(tempFile, targetFile, exe, arguments, canKill, processId); - return SendAsync(payload); + return SendAsync(payload, cancel); } public static string BuildMovePayload(string tempFile, string targetFile, string? exe, string? arguments, bool canKill, int? processId) @@ -91,13 +91,14 @@ static void Send(string payload) } } - static async Task SendAsync(string payload) + static async Task SendAsync(string payload, Cancel cancel) { try { - await InnerSendAsync(payload); + await InnerSendAsync(payload, cancel); } - catch (Exception exception) + // Let cancellation surface to the caller; only genuine send failures are swallowed. + catch (Exception exception) when (exception is not OperationCanceledException) { HandleSendException(payload, exception); } @@ -132,16 +133,28 @@ static void InnerSend(string payload) } } - static async Task InnerSendAsync(string payload) + static async Task InnerSendAsync(string payload, Cancel cancel) { using var client = new TcpClient(); var endpoint = GetEndpoint(); try { - await client.ConnectAsync(endpoint.Address, endpoint.Port); +#if NET6_0_OR_GREATER + await client.ConnectAsync(endpoint.Address, endpoint.Port, cancel); using var stream = client.GetStream(); using var writer = new StreamWriter(stream); - await writer.WriteAsync(payload); + await writer.WriteAsync(payload.AsMemory(), cancel); +#else + cancel.ThrowIfCancellationRequested(); + // Older frameworks lack cancellable Connect/Write, so abort by closing the client. + using (cancel.Register(client.Close)) + { + await client.ConnectAsync(endpoint.Address, endpoint.Port); + using var stream = client.GetStream(); + using var writer = new StreamWriter(stream); + await writer.WriteAsync(payload); + } +#endif } finally { diff --git a/src/DiffEngineTray.Tests/DiffRunnerCanKillTest.cs b/src/DiffEngineTray.Tests/DiffRunnerCanKillTest.cs new file mode 100644 index 00000000..f5b30342 --- /dev/null +++ b/src/DiffEngineTray.Tests/DiffRunnerCanKillTest.cs @@ -0,0 +1,118 @@ +using System.Net; +using System.Net.Sockets; +using DiffEngine; + +#pragma warning disable CS0618 // DiffEngineTray is obsolete; the test drives it directly to enable the send path. + +// Regression test for the sync launch path sending the wrong `canKill` value. +// MDI tools host every diff in one shared window, so the tray must never kill them (CanKill=false); +// non-MDI tools get their own process, which the tray may kill (CanKill=true). +public class DiffRunnerCanKillTest : + IDisposable +{ + string tempFile = Path.GetTempFileName(); + bool originalDisabled = DiffRunner.Disabled; + string? originalMaxInstances = Environment.GetEnvironmentVariable("DiffEngine_MaxInstances"); + + public DiffRunnerCanKillTest() + { + PiperClient.Port = GetFreePort(); + DiffEngine.DiffEngineTray.IsRunning = true; + DiffRunner.Disabled = false; + // Force the "too many running" branch so no real process is launched, while a move + // payload is still sent to the tray. The env var takes precedence over the app-domain + // value, so set it too; MaxInstancesToLaunch resets the cached lookup. + Environment.SetEnvironmentVariable("DiffEngine_MaxInstances", "0"); + DiffRunner.MaxInstancesToLaunch(0); + } + + [Test] + public async Task Sync_launch_of_mdi_tool_marks_move_as_not_killable() + { + var received = await CaptureMove(() => Task.FromResult(DiffRunner.Launch(MdiTool(), tempFile, "target.txt"))); + + await Assert.That(received.CanKill).IsFalse(); + } + + [Test] + public async Task Async_launch_of_mdi_tool_marks_move_as_not_killable() + { + var received = await CaptureMove(() => DiffRunner.LaunchAsync(MdiTool(), tempFile, "target.txt")); + + await Assert.That(received.CanKill).IsFalse(); + } + + [Test] + public async Task Sync_launch_of_non_mdi_tool_marks_move_as_killable() + { + var received = await CaptureMove(() => Task.FromResult(DiffRunner.Launch(NonMdiTool(), tempFile, "target.txt"))); + + await Assert.That(received.CanKill).IsTrue(); + } + + static async Task CaptureMove(Func> launch) + { + MovePayload? received = null; + var source = new CancelSource(); + var server = PiperServer.Start(move => received = move, _ => { }, source.Token); + try + { + var result = await launch(); + await Assert.That(result).IsEqualTo(LaunchResult.TooManyRunningDiffTools); + + for (var i = 0; received == null && i < 50; i++) + { + await Task.Delay(100); + } + } + finally + { + await source.CancelAsync(); + await server; + } + + await Assert.That(received).IsNotNull(); + return received!; + } + + static ResolvedTool MdiTool() => Tool(isMdi: true); + + static ResolvedTool NonMdiTool() => Tool(isMdi: false); + + static ResolvedTool Tool(bool isMdi) => + new( + name: "FakeCanKillTool", + exePath: Environment.ProcessPath!, + launchArguments: new( + Left: (temp, target) => $"\"{temp}\" \"{target}\"", + Right: (temp, target) => $"\"{target}\" \"{temp}\""), + isMdi: isMdi, + autoRefresh: false, + binaryExtensions: [], + requiresTarget: false, + supportsText: true, + useShellExecute: false); + + static int GetFreePort() + { + var probe = new TcpListener(IPAddress.Loopback, 0); + probe.Start(); + try + { + return ((IPEndPoint) probe.LocalEndpoint).Port; + } + finally + { + probe.Stop(); + } + } + + public void Dispose() + { + DiffEngine.DiffEngineTray.IsRunning = false; + DiffRunner.Disabled = originalDisabled; + Environment.SetEnvironmentVariable("DiffEngine_MaxInstances", originalMaxInstances); + DiffRunner.MaxInstancesToLaunch(5); + File.Delete(tempFile); + } +} diff --git a/src/DiffEngineTray.Tests/FileExTest.cs b/src/DiffEngineTray.Tests/FileExTest.cs new file mode 100644 index 00000000..440c99f2 --- /dev/null +++ b/src/DiffEngineTray.Tests/FileExTest.cs @@ -0,0 +1,31 @@ +public class FileExTest +{ + [Test] + public async Task SafeDeleteDirectoryRemovesDirectoryWithOnlyEmptySubDirectories() + { + var root = Path.Combine(Path.GetTempPath(), "DiffEngineSafeDelete", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(Path.Combine(root, "sub", "nested")); + + FileEx.SafeDeleteDirectory(root); + + await Assert.That(Directory.Exists(root)).IsFalse(); + } + + [Test] + public async Task SafeDeleteDirectoryKeepsDirectoryThatContainsAFile() + { + var root = Path.Combine(Path.GetTempPath(), "DiffEngineSafeDelete", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(root); + await File.WriteAllTextAsync(Path.Combine(root, "keep.txt"), "data"); + try + { + FileEx.SafeDeleteDirectory(root); + + await Assert.That(Directory.Exists(root)).IsTrue(); + } + finally + { + Directory.Delete(root, true); + } + } +} diff --git a/src/DiffEngineTray.Tests/PiperTest.SendOnly.verified.txt b/src/DiffEngineTray.Tests/PiperTest.SendOnly.verified.txt index 23ac00b0..06f3420c 100644 --- a/src/DiffEngineTray.Tests/PiperTest.SendOnly.verified.txt +++ b/src/DiffEngineTray.Tests/PiperTest.SendOnly.verified.txt @@ -14,9 +14,7 @@ Exception: System.Net.Sockets.SocketException (10061): No connection could be made because the target machine actively refused it. at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken) at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token) - at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state) ---- End of stack trace from previous location --- - at System.Net.Sockets.TcpClient.CompleteConnectAsync(Task task), + at System.Net.Sockets.TcpClient.CompleteConnectAsync(ValueTask task), Failed to send payload to DiffEngineTray. Payload: @@ -29,7 +27,5 @@ Exception: System.Net.Sockets.SocketException (10061): No connection could be made because the target machine actively refused it. at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.ThrowException(SocketError error, CancellationToken cancellationToken) at System.Net.Sockets.Socket.AwaitableSocketAsyncEventArgs.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token) - at System.Threading.Tasks.ValueTask.ValueTaskSourceAsTask.<>c.<.cctor>b__4_0(Object state) ---- End of stack trace from previous location --- - at System.Net.Sockets.TcpClient.CompleteConnectAsync(Task task) + at System.Net.Sockets.TcpClient.CompleteConnectAsync(ValueTask task) ] \ No newline at end of file diff --git a/src/DiffEngineTray.Tests/PiperTest.cs b/src/DiffEngineTray.Tests/PiperTest.cs index 20fe63db..a3dba172 100644 --- a/src/DiffEngineTray.Tests/PiperTest.cs +++ b/src/DiffEngineTray.Tests/PiperTest.cs @@ -84,6 +84,25 @@ public async Task Move() await Verify(received); } + [Test] + public async Task SendMoveAsyncHonorsCancellation() + { + using var source = new CancelSource(); + source.Cancel(); + + var cancelled = false; + try + { + await PiperClient.SendMoveAsync("Foo", "Bar", "theExe", "TheArguments", true, 10, source.Token); + } + catch (OperationCanceledException) + { + cancelled = true; + } + + await Assert.That(cancelled).IsTrue(); + } + [Test] public async Task ClientDisconnectsAbruptly() { diff --git a/src/DiffEngineTray.Tests/ProcessExTest.cs b/src/DiffEngineTray.Tests/ProcessExTest.cs index cdba52d7..6ff768de 100644 --- a/src/DiffEngineTray.Tests/ProcessExTest.cs +++ b/src/DiffEngineTray.Tests/ProcessExTest.cs @@ -14,4 +14,16 @@ public async Task TryGetMissing() await Assert.That(ProcessEx.TryGet(40000, out var found)).IsFalse(); await Assert.That(found).IsNull(); } + + [Test] + public async Task DescribeIsSafeForDisposedProcess() + { + var process = Process.GetCurrentProcess(); + process.Dispose(); + + // Id/MainModule can throw on a disposed process; Describe must swallow that. + var description = ProcessEx.Describe(process); + + await Assert.That(description).IsNotNull(); + } } diff --git a/src/DiffEngineTray.Tests/ProgramKeyBindingsTest.cs b/src/DiffEngineTray.Tests/ProgramKeyBindingsTest.cs new file mode 100644 index 00000000..3fa30880 --- /dev/null +++ b/src/DiffEngineTray.Tests/ProgramKeyBindingsTest.cs @@ -0,0 +1,40 @@ +public class ProgramKeyBindingsTest +{ + [Test] + public async Task Each_configured_hotkey_uses_a_distinct_binding_id() + { + var settings = new Settings + { + DiscardAllHotKey = new() { Key = "A" }, + AcceptAllHotKey = new() { Key = "B" }, + AcceptOpenHotKey = new() { Key = "C" }, + }; + await using var tracker = new RecordingTracker(); + + var ids = Program.BuildKeyBindings(settings, tracker) + .Select(_ => _.Id) + .ToList(); + + await Assert.That(ids.Count).IsEqualTo(3); + // A duplicate id would make KeyRegister unregister and overwrite an earlier hot key. + await Assert.That(ids.Distinct().Count()).IsEqualTo(3); + await Assert.That(ids.Contains(KeyBindingIds.DiscardAll)).IsTrue(); + await Assert.That(ids.Contains(KeyBindingIds.AcceptAll)).IsTrue(); + await Assert.That(ids.Contains(KeyBindingIds.AcceptOpen)).IsTrue(); + } + + [Test] + public async Task AcceptOpen_hotkey_maps_to_the_AcceptOpen_binding() + { + var settings = new Settings + { + AcceptOpenHotKey = new() { Key = "C" }, + }; + await using var tracker = new RecordingTracker(); + + var binding = Program.BuildKeyBindings(settings, tracker) + .Single(); + + await Assert.That(binding.Id).IsEqualTo(KeyBindingIds.AcceptOpen); + } +} diff --git a/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs b/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs index 44d924db..5dd56ca0 100644 --- a/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs +++ b/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs @@ -1,11 +1,39 @@ -#if DEBUG public class SolutionDirectoryFinderTests { +#if DEBUG static string SourceFile { get; } = GetSourceFile(); static string GetSourceFile([CallerFilePath] string path = "") => path; [Test] public Task Find() => Verify(SolutionDirectoryFinder.Find(SourceFile)); -} #endif + + [Test] + public async Task SiblingWithSharedPrefixIsNotMatched() + { + var root = Path.Combine(Path.GetTempPath(), "DiffEngineSlnFinder", Guid.NewGuid().ToString("N")); + var appDir = Path.Combine(root, "App"); + var appTestsDir = Path.Combine(root, "AppTests"); + Directory.CreateDirectory(appDir); + Directory.CreateDirectory(appTestsDir); + try + { + await File.WriteAllTextAsync(Path.Combine(appDir, "App.sln"), ""); + await File.WriteAllTextAsync(Path.Combine(appTestsDir, "AppTests.sln"), ""); + + // Resolve a file inside App first so its directory gets cached. + var appResult = SolutionDirectoryFinder.Find(Path.Combine(appDir, "Class.cs")); + await Assert.That(appResult).IsEqualTo("App"); + + // A sibling that merely shares the "App" name prefix must resolve to its own + // solution, not to the cached "App" directory. + var testsResult = SolutionDirectoryFinder.Find(Path.Combine(appTestsDir, "Tests.cs")); + await Assert.That(testsResult).IsEqualTo("AppTests"); + } + finally + { + Directory.Delete(root, true); + } + } +} diff --git a/src/DiffEngineTray/FileEx.cs b/src/DiffEngineTray/FileEx.cs index 84113b77..d0ad1009 100644 --- a/src/DiffEngineTray/FileEx.cs +++ b/src/DiffEngineTray/FileEx.cs @@ -2,8 +2,8 @@ static class FileEx { - public static bool IsEmptyDirectory(string directory) => - !Directory.EnumerateFiles(directory, "*", SearchOption.AllDirectories).Any(); + public static bool ContainsFiles(string directory) => + Directory.EnumerateFiles(directory, "*", SearchOption.AllDirectories).Any(); public static bool SafeDeleteFile(string path) { @@ -37,14 +37,16 @@ public static void SafeDeleteDirectory(string path) return; } - if (!IsEmptyDirectory(path)) + // Leave the directory if it still holds files (a running test may be using them). + // Otherwise it is safe to remove the whole tree, including any empty sub-directories. + if (ContainsFiles(path)) { return; } try { - Directory.Delete(path, false); + Directory.Delete(path, true); } catch (IOException exception) { diff --git a/src/DiffEngineTray/ProcessEx.cs b/src/DiffEngineTray/ProcessEx.cs index bb8fa7e3..454d0304 100644 --- a/src/DiffEngineTray/ProcessEx.cs +++ b/src/DiffEngineTray/ProcessEx.cs @@ -31,13 +31,16 @@ public static bool TryGet(int id, [NotNullWhen(true)] out Process? process) public static void KillAndDispose(this Process process) { + // Capture identity up front. Once the process has exited, Id/MainModule can throw, + // so reading them in the error handlers below could mask the real failure. + var description = Describe(process); try { process.Kill(); var exited = process.WaitForExit(500); if (!exited) { - ExceptionHandler.Handle($"Failed to kill process. Id:{process.Id} Name: {process.MainModule?.FileName}"); + ExceptionHandler.Handle($"Failed to kill process. {description}"); } } catch (InvalidOperationException) @@ -51,11 +54,24 @@ public static void KillAndDispose(this Process process) } catch (Exception exception) { - ExceptionHandler.Handle($"Failed to kill process. Id:{process.Id} Name: {process.MainModule?.FileName}", exception); + ExceptionHandler.Handle($"Failed to kill process. {description}", exception); } finally { process.Dispose(); } } + + internal static string Describe(Process process) + { + try + { + return $"Id:{process.Id} Name: {process.MainModule?.FileName}"; + } + catch (Exception) + { + // Id/MainModule can throw for an exited, disposed or inaccessible process. + return "Id: unknown"; + } + } } \ No newline at end of file diff --git a/src/DiffEngineTray/Program.cs b/src/DiffEngineTray/Program.cs index c3aac851..f4a01bcc 100644 --- a/src/DiffEngineTray/Program.cs +++ b/src/DiffEngineTray/Program.cs @@ -84,45 +84,43 @@ static async Task Inner() [UnsafeAccessor(UnsafeAccessorKind.Method, Name = "ShowContextMenu")] static extern void ShowContextMenu(NotifyIcon icon); - static void ReBindKeys(Settings settings, KeyRegister keyRegister, Tracker tracker) + internal static void ReBindKeys(Settings settings, KeyRegister keyRegister, Tracker tracker) { - var discardAllHotKey = settings.DiscardAllHotKey; - if (discardAllHotKey != null) + foreach (var binding in BuildKeyBindings(settings, tracker)) { + var hotKey = binding.HotKey; keyRegister.TryAddBinding( - KeyBindingIds.DiscardAll, - discardAllHotKey.Shift, - discardAllHotKey.Control, - discardAllHotKey.Alt, - discardAllHotKey.Key, - tracker.Clear); + binding.Id, + hotKey.Shift, + hotKey.Control, + hotKey.Alt, + hotKey.Key, + binding.Action); } + } - var acceptAllHotKey = settings.AcceptAllHotKey; - if (acceptAllHotKey != null) + // Each configured hot key must map to a distinct KeyBindingIds value. + // Reusing an id causes KeyRegister.TryAddBinding to unregister and overwrite the earlier binding. + internal static IEnumerable BuildKeyBindings(Settings settings, Tracker tracker) + { + if (settings.DiscardAllHotKey is { } discardAll) { - keyRegister.TryAddBinding( - KeyBindingIds.AcceptAll, - acceptAllHotKey.Shift, - acceptAllHotKey.Control, - acceptAllHotKey.Alt, - acceptAllHotKey.Key, - tracker.AcceptAll); + yield return new(KeyBindingIds.DiscardAll, discardAll, tracker.Clear); } - var acceptOpenHotKey = settings.AcceptOpenHotKey; - if (acceptOpenHotKey != null) + if (settings.AcceptAllHotKey is { } acceptAll) { - keyRegister.TryAddBinding( - KeyBindingIds.AcceptAll, - acceptOpenHotKey.Shift, - acceptOpenHotKey.Control, - acceptOpenHotKey.Alt, - acceptOpenHotKey.Key, - tracker.AcceptOpen); + yield return new(KeyBindingIds.AcceptAll, acceptAll, tracker.AcceptAll); + } + + if (settings.AcceptOpenHotKey is { } acceptOpen) + { + yield return new(KeyBindingIds.AcceptOpen, acceptOpen, tracker.AcceptOpen); } } + internal record KeyBinding(int Id, HotKey HotKey, Action Action); + static async Task GetSettings() { try diff --git a/src/DiffEngineTray/SolutionDirectoryFinder.cs b/src/DiffEngineTray/SolutionDirectoryFinder.cs index 72fc295e..e7cbfe2a 100644 --- a/src/DiffEngineTray/SolutionDirectoryFinder.cs +++ b/src/DiffEngineTray/SolutionDirectoryFinder.cs @@ -13,14 +13,29 @@ class Result(string directory, string name) static Result? Inner(string file) { - foreach (var result in cache.Values.Where(_ => _ != null)) + // Reuse an already resolved solution when the file sits inside its directory. + // Prefer the nearest (longest) enclosing directory so nested solutions resolve correctly. + Result? nearest = null; + foreach (var result in cache.Values) { - if (file.StartsWith(result!.Directory)) + if (result == null || + !IsInDirectory(file, result.Directory)) { - return result; + continue; + } + + if (nearest == null || + result.Directory.Length > nearest.Directory.Length) + { + nearest = result; } } + if (nearest != null) + { + return nearest; + } + var currentDirectory = Path.GetDirectoryName(file); if (string.IsNullOrEmpty(currentDirectory)) { @@ -49,6 +64,25 @@ class Result(string directory, string name) } while (true); } + // True when file is directory itself or sits below it, requiring a directory-separator + // boundary so that a sibling like "AppTests" is not treated as being inside "App". + static bool IsInDirectory(string file, string directory) + { + if (!file.StartsWith(directory, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + if (file.Length == directory.Length) + { + return true; + } + + var boundary = file[directory.Length]; + return boundary == Path.DirectorySeparatorChar || + boundary == Path.AltDirectorySeparatorChar; + } + static bool TryFind(string directory, string searchPattern, [NotNullWhen(true)] out Result? result) { var solutions = Directory.GetFiles(directory, searchPattern);