diff --git a/src/DiffEngine.Tests/CsStringLiteralTests.cs b/src/DiffEngine.Tests/CsStringLiteralTests.cs index f5af28cf..2a7b3aa5 100644 --- a/src/DiffEngine.Tests/CsStringLiteralTests.cs +++ b/src/DiffEngine.Tests/CsStringLiteralTests.cs @@ -22,9 +22,21 @@ public class CsStringLiteralTests "a\n \nb", "trailing space \nnext", "emoji 🎈 and unicode β˜‚", - "line1\n indented\nline3" + "line1\n indented\nline3", + "x" + nextLine + "y", + "x" + lineSeparator + "y", + "x" + paragraphSeparator + "y", + "a\nb" + lineSeparator + "c", + lineSeparator + "leading", + "trailing" + paragraphSeparator ]; + // Line terminators to the C# lexer, past \n and \r. Named by code point rather than written + // into a literal, so nothing between here and the compiler can normalize them away + const char nextLine = (char) 0x85; + const char lineSeparator = (char) 0x2028; + const char paragraphSeparator = (char) 0x2029; + [Test] public async Task RenderSimple() { @@ -61,6 +73,50 @@ public async Task RenderQuoteRunEscalatesDelimiter() await Assert.That(rendered).IsEqualTo("\"\"\"\"\nhas \"\"\" inside\n\"\"\"\""); } + [Test] + [Arguments(0x85)] + [Arguments(0x2028)] + [Arguments(0x2029)] + public async Task RenderEscapesLineTerminator(int codePoint) + { + var terminator = (char) codePoint; + var rendered = CsStringLiteral.Render($"x{terminator}y", " ", "\n"); + + // Carried as an escape. Left as itself it ends the literal, and the file stops compiling + // (CS1010) + await Assert.That(rendered).IsEqualTo($"\"x\\u{codePoint:x4}y\""); + await Assert.That(rendered).DoesNotContain(terminator.ToString()); + } + + [Test] + [Arguments(0x85)] + [Arguments(0x2028)] + [Arguments(0x2029)] + public async Task RenderFallsBackToRegularWhenRawCannotHoldLineTerminator(int codePoint) + { + var terminator = (char) codePoint; + var content = $"a\nb{terminator}c"; + var rendered = CsStringLiteral.Render(content, " ", "\n"); + + // Content over several lines is otherwise raw, but no delimiter width makes a raw string + // able to hold this: the compiler reads the terminator as a line break, and the line it + // starts does not carry the closing delimiter's indentation (CS8999) + await Assert.That(rendered.StartsWith("\"\"\"", StringComparison.Ordinal)).IsFalse(); + await Assert.That(rendered).DoesNotContain(terminator.ToString()); + await Assert.That(CsStringLiteral.TryParse(rendered, out var value)).IsTrue(); + await Assert.That(value).IsEqualTo(content); + } + + [Test] + public async Task RenderRawFallsBackWhenContentHoldsLineTerminator() + { + // The fallback sits in RenderRaw rather than in Render, so asking for the raw form + // directly still produces source that compiles + var content = $"a\nb{lineSeparator}c"; + await Assert.That(CsStringLiteral.RenderRaw(content, " ", "\n")) + .IsEqualTo(StringLiteral.RenderRegular(content)); + } + [Test] [Arguments("a\r\nb")] [Arguments("a\rb")] @@ -173,6 +229,32 @@ public async Task Parse(string expression, string expected) await Assert.That(value).IsEqualTo(expected); } + // A verbatim literal opening on an escaped quote. The quote run is not a delimiter here, and + // reading it as one rejected a literal the compiler accepts + [Test] + [Arguments("@\"\"\"x\"\"\"", "\"x\"")] + [Arguments("@\"\"\"\"", "\"")] + [Arguments("@\"\"\"a\"\"b\"\"\"", "\"a\"b\"")] + public async Task ParseVerbatimOpeningOnQuote(string expression, string expected) + { + var parsed = CsStringLiteral.TryParse(expression, out var value); + await Assert.That(parsed).IsTrue(); + await Assert.That(value).IsEqualTo(expected); + } + + // Half a surrogate pair, and a code point past the Unicode range. ConvertFromUtf32 throws on + // both, and a throw here reaches the process applying the patch + [Test] + [Arguments("\"\\U0000D800\"")] + [Arguments("\"\\U0000DFFF\"")] + [Arguments("\"\\U00110000\"")] + [Arguments("\"\\UFFFFFFFF\"")] + public async Task ParseRejectsUnreadableCodePoint(string expression) + { + var parsed = CsStringLiteral.TryParse(expression, out _); + await Assert.That(parsed).IsFalse(); + } + [Test] public async Task ParseMultiLineVerbatim() { diff --git a/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs b/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs index 34dc7f53..e94fd564 100644 --- a/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs +++ b/src/DiffEngine.Tests/FsCompilerRoundTripTests.cs @@ -45,7 +45,14 @@ public class FsCompilerRoundTripTests "has \"\"\" inside\nsecond", "(* not a comment *)\nsecond", "// not a comment\nsecond", - "'ticked'\nsecond" + "'ticked'\nsecond", + // Line terminators, which are rendered as escapes rather than written into a + // triple-quoted literal. What is being asked of fsi is that it reads the escape back as + // the one character, since nothing on this side can tell whether it did + "next line" + (char) 0x85 + "inside", + "separator" + (char) 0x2028 + "inside", + "paragraph" + (char) 0x2029 + "inside", + "a\nb" + (char) 0x2028 + "c" ]; [Test] diff --git a/src/DiffEngine.Tests/FsStringLiteralTests.cs b/src/DiffEngine.Tests/FsStringLiteralTests.cs index 701457dc..e6841543 100644 --- a/src/DiffEngine.Tests/FsStringLiteralTests.cs +++ b/src/DiffEngine.Tests/FsStringLiteralTests.cs @@ -25,9 +25,17 @@ public class FsStringLiteralTests "line1\n indented\nline3", "back\\slash\nsecond", "tab\there\nsecond", - "{\n \"name\": \"value\"\n}" + "{\n \"name\": \"value\"\n}", + "x" + lineSeparator + "y", + "a\nb" + lineSeparator + "c", + "a\nb" + nextLine + "c", + "a\nb" + paragraphSeparator + "c" ]; + const char nextLine = (char) 0x85; + const char lineSeparator = (char) 0x2028; + const char paragraphSeparator = (char) 0x2029; + // The same shape C# writes: the content indented under the call, with the first line and the // closing delimiter's indentation there to be taken back off [Test] @@ -56,6 +64,24 @@ public async Task RenderMultiLineMatchesCSharp() } } + [Test] + [Arguments(0x85)] + [Arguments(0x2028)] + [Arguments(0x2029)] + public async Task RenderFallsBackToRegularWhenContentHoldsLineTerminator(int codePoint) + { + var terminator = (char) codePoint; + var content = $"a\nb{terminator}c"; + var rendered = FsStringLiteral.Render(content, " ", "\n"); + + // Kept off the triple-quoted form for the reason C# is: the two write the one shape, and + // that is what RenderMultiLineMatchesCSharp is asserting over these same cases + await Assert.That(rendered.StartsWith("\"\"\"", StringComparison.Ordinal)).IsFalse(); + await Assert.That(rendered).DoesNotContain(terminator.ToString()); + await Assert.That(FsStringLiteral.TryParse(rendered, out var value)).IsTrue(); + await Assert.That(value).IsEqualTo(content); + } + [Test] public async Task RenderBlankLineHasNoTrailingWhitespace() { @@ -213,6 +239,19 @@ public async Task Parse(string expression, string expected) await Assert.That(value).IsEqualTo(expected); } + // Half a surrogate pair, and a code point past the Unicode range. ConvertFromUtf32 throws on + // both, and a throw here reaches the process applying the patch + [Test] + [Arguments("\"\\U0000D800\"")] + [Arguments("\"\\U0000DFFF\"")] + [Arguments("\"\\U00110000\"")] + [Arguments("\"\\UFFFFFFFF\"")] + public async Task ParseRejectsUnreadableCodePoint(string expression) + { + var parsed = FsStringLiteral.TryParse(expression, out _); + await Assert.That(parsed).IsFalse(); + } + [Test] public async Task ParseMultiLineTakesTheLayoutOff() { diff --git a/src/DiffEngine.Tests/GlobalUsings.cs b/src/DiffEngine.Tests/GlobalUsings.cs index 1a351c1f..49fe999c 100644 --- a/src/DiffEngine.Tests/GlobalUsings.cs +++ b/src/DiffEngine.Tests/GlobalUsings.cs @@ -1,4 +1,5 @@ global using EmptyFiles; +global using System.Collections.Concurrent; global using System.Diagnostics; global using System.Reflection; global using System.Text; diff --git a/src/DiffEngine.Tests/InlineApplierTests.cs b/src/DiffEngine.Tests/InlineApplierTests.cs index 310ad86c..4db3df15 100644 --- a/src/DiffEngine.Tests/InlineApplierTests.cs +++ b/src/DiffEngine.Tests/InlineApplierTests.cs @@ -1,4 +1,4 @@ -ο»Ώpublic class InlineApplierTests +public class InlineApplierTests { static string WriteTemp(byte[] bytes, string extension = ".cs") { @@ -7,6 +7,14 @@ static string WriteTemp(byte[] bytes, string extension = ".cs") return path; } + // A directory of its own, for the tests that are about what is left in one + static string NewDirectory() + { + var path = Path.Combine(Path.GetTempPath(), $"InlineApplierTests_{Guid.NewGuid():N}"); + Directory.CreateDirectory(path); + return path; + } + static byte[] Utf8(string text, bool bom) { var encoding = new UTF8Encoding(bom); @@ -95,6 +103,146 @@ public async Task Utf16Preserved() } } + // The swap goes through a sibling temporary. It is gone by the time the patch is applied, + // whatever else is true, because a stray file in a source directory is the caller's problem + [Test] + public async Task LeavesNoTemporaryBehind() + { + var directory = NewDirectory(); + try + { + var path = Path.Combine(directory, "Sample.cs"); + File.WriteAllBytes(path, Utf8(source, bom: false)); + + var result = InlineApplier.Apply(Patch(path, 3, "\"old\"", "new")); + + await Assert.That(result.Status).IsEqualTo(InlineApplyStatus.Applied); + await Assert.That(Directory.GetFileSystemEntries(directory)).IsEquivalentTo([path]); + } + finally + { + Directory.Delete(directory, true); + } + } + + // Writing in place truncates first, so a reader - or a process that stops partway, which is + // the case this stands in for - could see a file with its tail missing. Reading alongside the + // apply can only fail when that window is real, so it never goes red on timing alone + [Test] + public async Task ContentIsNeverObservedHalfWritten() + { + var padding = string.Join("\n", Enumerable.Repeat(" // padding, to widen the window a truncating write would open", 30000)); + var text = $"class C\n{{\n void M() => Verify(value).Snapshot(\"old\");\n{padding}\n}}"; + var directory = NewDirectory(); + try + { + var path = Path.Combine(directory, "Big.cs"); + File.WriteAllBytes(path, Utf8(text, bom: false)); + var before = new FileInfo(path).Length; + + using var cancellation = new CancellationTokenSource(); + var seen = new ConcurrentDictionary(); + var reader = Task.Run( + () => + { + while (!cancellation.IsCancellationRequested) + { + try + { + // Shared every way, so watching the file never blocks the write being + // watched. Holding it any other way tests which of the two wins the + // file rather than what a reader of it sees + using var stream = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.ReadWrite | FileShare.Delete); + seen.TryAdd(stream.Length, 0); + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + // The swap is in flight. Not an observation of the content + } + } + }); + + // Long enough that the two whole files differ in length, which is what makes a + // half written one tell itself apart + var result = InlineApplier.Apply(Patch(path, 3, "\"old\"", "a replacement longer than what it replaces")); + cancellation.Cancel(); + await reader; + + await Assert.That(result.Status).IsEqualTo(InlineApplyStatus.Applied); + var after = new FileInfo(path).Length; + await Assert.That(after).IsNotEqualTo(before); + + var partial = seen.Keys.Where(_ => _ != before && _ != after).ToList(); + await Assert.That(partial).IsEmpty(); + } + finally + { + Directory.Delete(directory, true); + } + } + + // Swapping the file in takes the path away for the instant it takes to rename over it. An + // applier waiting its turn must not read that as a file that is not there, which is what + // asking before taking the lock did + [Test] + public async Task ConcurrentAppliesNeverReportAMissingFile() + { + const int writers = 4; + const int rounds = 60; + var directory = NewDirectory(); + try + { + // A snapshot each, flipped back and forth, so every apply is a real write and the + // file is swapped out from under the others hundreds of times. The window this is + // about is microseconds wide and no arrangement here reproduces it on demand - it + // showed up as ParallelAppliesToSameFile going red under the load of the rest of this + // class. What is pinned is the contract: contended appliers all get their write, and + // none of them reports the file as gone + var methods = Enumerable.Range(0, writers) + .Select(_ => $" void M{_}() => Verify(v).Snapshot(\"a{_}\");"); + var path = Path.Combine(directory, "Contended.cs"); + File.WriteAllBytes(path, Utf8($"class C\n{{\n{string.Join("\n", methods)}\n}}", bom: false)); + + var results = await Task.WhenAll( + Enumerable + .Range(0, writers) + .Select( + index => Task.Run( + () => + { + var outcomes = new List(); + for (var round = 0; round < rounds; round++) + { + var (from, to) = round % 2 == 0 + ? ($"a{index}", $"b{index}") + : ($"b{index}", $"a{index}"); + outcomes.Add(InlineApplier.Apply(Patch(path, index + 3, $"\"{from}\"", to))); + } + + return outcomes; + }))); + + var failures = results + .SelectMany(_ => _) + .Where(_ => _.Status == InlineApplyStatus.Failed) + .Select(_ => _.Message) + .ToList(); + await Assert.That(failures).IsEmpty(); + + // And none of them lost another's line along the way + var text = await File.ReadAllTextAsync(path); + for (var index = 0; index < writers; index++) + { + await Assert.That(text).Contains($"void M{index}()"); + } + } + finally + { + Directory.Delete(directory, true); + } + } + // The whole file is rewritten, not just the patched span, so a byte that does not decode // would come back as a replacement character everywhere it appears [Test] @@ -540,6 +688,50 @@ public async Task UnknownModeFails() await Assert.That(read).IsFalse(); } + // A number parses as an enum as readily as a name, so this used to arrive as an + // InlinePatchMode that is none of them and behave as a Set + [Test] + [Arguments("7")] + [Arguments("-1")] + [Arguments("99")] + public async Task UndefinedNumericModeFails(string mode) + { + var read = InlinePatchFile.TryParse($"version: 2\nsourceFile: x\nlineHint: 1\nmode: {mode}\noriginalExpression:\nnewContent: YQ==\n", out _); + await Assert.That(read).IsFalse(); + } + + // The numbers that do name a mode still read, since that is how the enum has always been + // written on the wire + [Test] + public async Task DefinedNumericModeReads() + { + var read = InlinePatchFile.TryParse($"version: 2\nsourceFile: x\nlineHint: 1\nmode: {(int) InlinePatchMode.Append}\noriginalExpression:\nnewContent: YQ==\n", out var patch); + await Assert.That(read).IsTrue(); + await Assert.That(patch!.Mode).IsEqualTo(InlinePatchMode.Append); + } + + // Both ride the payload as themselves, so a line break in either ends the line and the rest + // is read as more of the payload: the fixed lines shift, or a trailing field is forged + [Test] + public async Task ALineBreakInAFieldThatIsNotEncodedIsRefused() + { + await Assert.That( + () => InlinePatchFile.Build( + new("a\nb.cs", 1, null, "new", InlinePatchMode.Set) + { + TestName = null + })) + .Throws(); + await Assert.That( + () => InlinePatchFile.Build( + new("a.cs", 1, null, "new", InlinePatchMode.Set) + { + TestName = null, + Framework = "net8.0\noriginalValue: forged" + })) + .Throws(); + } + [Test] public async Task MissingFileFails() { diff --git a/src/DiffEngine.Tests/InlinePatcherTests.cs b/src/DiffEngine.Tests/InlinePatcherTests.cs index 3075c91b..60df1557 100644 --- a/src/DiffEngine.Tests/InlinePatcherTests.cs +++ b/src/DiffEngine.Tests/InlinePatcherTests.cs @@ -31,6 +31,52 @@ public async Task ReplaceRawLiteral() await Assert.That(newSource.EndsWith(");\n }\n}")).IsTrue(); } + /// + /// An interpolated raw string whose hole holds a literal of its own. Skipping holes as content + /// ended the outer string at the inner delimiter, after which the rest of the line lexed as + /// code and a stray delimiter opened a string that ran on and swallowed the real call. + /// + [Test] + public async Task ARawInterpolatedStringWithALiteralInItsHoleIsSteppedOverWhole() + { + var source = Method($"{rawInterpolated}\n await Snapshot(\"old\");"); + var status = TryApply(source, 6, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out var reason); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(reason).IsEmpty(); + await Assert.That(newSource).Contains("Snapshot(\"new\")"); + // And the literal it stepped over is untouched + await Assert.That(newSource).Contains(rawInterpolated); + } + + /// + /// The same shape with the call before it, so the scan has to get past the literal rather than + /// stop short of it. + /// + [Test] + public async Task ASnapshotBeforeARawInterpolatedStringIsStillFound() + { + var source = Method($" await Snapshot(\"old\");\n{rawInterpolated}"); + var status = TryApply(source, 5, InlinePatchMode.Set, "\"old\"", "new", out var newSource, out _); + + await Assert.That(status).IsEqualTo(PatchStatus.Applied); + await Assert.That(newSource).Contains("Snapshot(\"new\")"); + } + + // Written by concatenation because it holds runs of three and four quotes, which no raw string + // in this file can carry without a wider delimiter than the thing being described. + // + // The literal in the hole is four-quoted and holds a run of three, so the runs no longer pair + // up evenly. That matters: with a hole skipped as content, the blind scan ends the outer + // string at the first run of three or more and then re-pairs the rest, which for evenly + // matched runs lands back on its feet and hides the bug. Here the last run opens a string that + // runs to the end of the file, taking the Snapshot call with it + const string rawInterpolated = + " var text = $" + q3 + "{Render(" + q4 + "has " + q3 + " inside" + q4 + ")}" + q3 + ";"; + + const string q3 = "\"\"\""; + const string q4 = "\"\"\"\""; + [Test] public async Task ReplaceRegularLiteral() { diff --git a/src/DiffEngine.Tests/InlineQueueClientTests.cs b/src/DiffEngine.Tests/InlineQueueClientTests.cs index d938a7b7..18cbb237 100644 --- a/src/DiffEngine.Tests/InlineQueueClientTests.cs +++ b/src/DiffEngine.Tests/InlineQueueClientTests.cs @@ -183,6 +183,45 @@ public async Task AcceptOfAnUnknownCallSiteDoesNothing() await Assert.That(InlineQueueClient.Find(InlineKey.For("Sample.cs", 42))).IsNotNull(); } + /// + /// Whether the entry survived is what says an accept applied, and that takes a second round + /// trip. When it cannot be made β€” the owner exited, or answered with an error β€” there is no + /// answer to give, and the one that used to be given was Accepted: an empty item list read as + /// an empty queue, so a surface stopped offering a snapshot nothing had confirmed. + /// + [Test] + public async Task AnAcceptThatCannotBeConfirmedIsNotCalledAccepted() + { + using var owner = new Owner(); + owner.Enqueue(Patch()); + owner.ListingFails = true; + + var outcome = InlineQueueClient.Accept(InlineKey.For("Sample.cs", 42), out _); + + await Assert.That(outcome).IsEqualTo(InlineAcceptOutcome.Unknown); + // The owner did carry it out; only the confirmation was unavailable + await Assert.That(owner.Applied.Single().NewContent).IsEqualTo("new content"); + } + + /// + /// The same shape one level down: an error listing is not a statement that nothing is pending, + /// so a caller that falls back when no owner answered has to fall back here too. + /// + [Test] + public async Task AnErrorListingIsNotAnEmptyQueue() + { + using var owner = new Owner + { + ListingFails = true + }; + owner.Enqueue(Patch()); + + await Assert.That(InlineQueueClient.TryList(out var pending)).IsFalse(); + await Assert.That(pending).IsEmpty(); + await Assert.That(InlineQueueClient.TryListKeys(out var keys)).IsFalse(); + await Assert.That(keys).IsEmpty(); + } + [Test] public async Task DiscardDropsWithoutApplying() { @@ -317,8 +356,19 @@ void IQueueOwner.TrackDelete(string file) { } + /// + /// When set, every listing comes back as an error, which is how an owner that cannot say + /// what it holds is arranged without racing its own shutdown. + /// + public bool ListingFails { get; set; } + ViewerResponse IQueueOwner.Listing(bool withPatches) { + if (ListingFails) + { + return ViewerResponse.Error("the owner is going away"); + } + lock (gate) { return ViewerResponse.Listing(ViewerListing.Items(queue.Items, withPatches)); diff --git a/src/DiffEngine.Tests/InlineQueueTests.cs b/src/DiffEngine.Tests/InlineQueueTests.cs index 9b68a84c..7f976b02 100644 --- a/src/DiffEngine.Tests/InlineQueueTests.cs +++ b/src/DiffEngine.Tests/InlineQueueTests.cs @@ -181,6 +181,60 @@ public async Task CompletingAfterAReplaceLeavesTheNewEntry() await Assert.That(after.Items[0].Patch.NewContent).IsEqualTo("second"); } + /// + /// The other side of . Applying takes up + /// to ten seconds on the cross process mutex, and a test that is still failing re-runs and + /// re-sends the identical patch inside that window. That is not a replace: the entry says what + /// it said before, so the accept it is in the middle of still completes. + /// + /// Rebuilding the entry regardless made every one of those look like a change, so the patch + /// reached the file and the entry stayed pending with nothing said about it. + /// + /// + [Test] + public async Task CompletingAfterAnIdenticalReRunStillApplies() + { + var queue = InlineQueue.Empty.Enqueue(Patch(content: "same")); + var entry = queue.Find(InlineKey.For("Sample.cs", 42))!; + queue = queue.Enqueue(Patch(content: "same")); + + var after = queue.Accept(entry, InlineApplyResult.Applied, out var message); + + await Assert.That(message).IsEqualTo("Applied Sample.cs:42"); + await Assert.That(after.Items).IsEmpty(); + } + + /// + [Test] + public async Task CompletingAfterAnIdenticalReRunFromTheSameFrameworkStillApplies() + { + var queue = InlineQueue.Empty.Enqueue(Patch(content: "same", framework: "net8.0")); + var entry = queue.Find(InlineKey.For("Sample.cs", 42))!; + queue = queue.Enqueue(Patch(content: "same", framework: "net8.0")); + + var after = queue.Accept(entry, InlineApplyResult.Applied, out var message); + + await Assert.That(message).IsEqualTo("Applied Sample.cs:42"); + await Assert.That(after.Items).IsEmpty(); + } + + /// + /// A re-run still drops what the last attempt failed with, which is what rebuilding the entry + /// did: the content has arrived again and nothing has retried it. + /// + [Test] + public async Task AnIdenticalReRunClearsTheFailureStatus() + { + var queue = InlineQueue.Empty.Enqueue(Patch(content: "same")); + var entry = queue.Find(InlineKey.For("Sample.cs", 42))!; + queue = queue.Accept(entry, InlineApplyResult.Failed("the file is locked"), out _); + await Assert.That(queue.Items.Single().Status).IsEqualTo("the file is locked"); + + queue = queue.Enqueue(Patch(content: "same")); + + await Assert.That(queue.Items.Single().Status).IsNull(); + } + [Test] public async Task CompletingAfterASettleDoesNothing() { diff --git a/src/DiffEngine.Tests/ViewerProtocolTests.cs b/src/DiffEngine.Tests/ViewerProtocolTests.cs index 38ef40a2..64bf1818 100644 --- a/src/DiffEngine.Tests/ViewerProtocolTests.cs +++ b/src/DiffEngine.Tests/ViewerProtocolTests.cs @@ -97,7 +97,8 @@ public async Task EveryVerbRoundTrips() [Test] public async Task SettleCarriesTheKey() { - var payload = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("Tests.cs", 42)).Build(); + // Already lower case, so what the key survives is the round trip rather than the folding + var payload = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("tests.cs", 42)).Build(); await Assert.That(ViewerMessage.TryParse(payload, out var message)).IsTrue(); await Assert.That(message!.Verb).IsEqualTo(ViewerVerb.Settle); @@ -108,14 +109,39 @@ public async Task SettleCarriesTheKey() /// Settling only works if the sender and the queue owner derive the same key from the same /// call site, so the format is pinned rather than left to whatever ToLower happens to do. /// + /// These are already lower case, so they say the same thing wherever they run. [Test] + [Arguments("tests.cs", 42, "tests.cs|42")] + [Arguments(@"c:\repo\some.tests\sample.cs", 1, @"c:\repo\some.tests\sample.cs|1")] + [Arguments("/home/user/sample.cs", 9999, "/home/user/sample.cs|9999")] + public async Task KeyFormat(string sourceFile, int line, string expected) => + await Assert.That(InlineKey.For(sourceFile, line)).IsEqualTo(expected); + + /// + /// A Windows path reaches here from several sources with several casings, and every one of + /// them is the same call site. + /// + [Test] + [RunOn(TUnit.Core.Enums.OS.Windows)] [Arguments("Tests.cs", 42, "tests.cs|42")] [Arguments(@"C:\Repo\Some.Tests\Sample.cs", 1, @"c:\repo\some.tests\sample.cs|1")] - [Arguments("/home/user/Sample.cs", 9999, "/home/user/sample.cs|9999")] [Arguments("MiXeDCase.CS", 7, "mixedcase.cs|7")] - public async Task KeyFormat(string sourceFile, int line, string expected) => + public async Task KeyIsFoldedWhereThePathsAre(string sourceFile, int line, string expected) => await Assert.That(InlineKey.For(sourceFile, line)).IsEqualTo(expected); + /// + /// And not where they are not. On Linux these are two files, and one key for both meant the + /// second patch took over the first's entry and settling either settled both. + /// + [Test] + [RunOn(TUnit.Core.Enums.OS.Linux)] + public async Task KeysDifferingOnlyInCaseStayApartWhereTheFilesDo() + { + await Assert.That(InlineKey.For("/home/user/Sample.cs", 1)).IsEqualTo("/home/user/Sample.cs|1"); + await Assert.That(InlineKey.For("/home/user/sample.cs", 1)) + .IsNotEqualTo(InlineKey.For("/home/user/Sample.cs", 1)); + } + /// /// A newer sender can add a field without breaking an older owner. /// @@ -219,7 +245,7 @@ public async Task MetadataRidesTheInlineBody() [Test] public async Task SettleCarriesTheOriginInTheBody() { - var payload = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("Tests.cs", 42), "net9.0").Build(); + var payload = new ViewerMessage(ViewerVerb.Settle, InlineKey.For("tests.cs", 42), "net9.0").Build(); await Assert.That(ViewerMessage.TryParse(payload, out var message)).IsTrue(); await Assert.That(message!.Key).IsEqualTo("tests.cs|42"); diff --git a/src/DiffEngine/Inline/CsLanguage.cs b/src/DiffEngine/Inline/CsLanguage.cs index 313d3f1f..cf09d69c 100644 --- a/src/DiffEngine/Inline/CsLanguage.cs +++ b/src/DiffEngine/Inline/CsLanguage.cs @@ -210,8 +210,11 @@ static bool TrySkipStringLike(string source, ref int index) var quotes = QuoteRun(source, cursor); if (quotes >= 3) { - // Raw string (interpolated or not): skip blindly to a closing run of >= quotes. - // Interpolation holes are skipped as part of the content. + // Raw string: scan to a closing run of >= quotes, stepping over any interpolation + // hole whole. Skipping holes as content read the quotes of a literal inside one - + // $"""{Render("""x""")}""" - as the end of the outer string, after which the rest of + // the line lexed as code and a stray delimiter opened a string that could swallow a + // real call var search = cursor + quotes; while (true) { @@ -221,7 +224,28 @@ static bool TrySkipStringLike(string source, ref int index) return true; } - if (source[search] != '"') + var ch = source[search]; + if (dollars > 0 && ch == '{') + { + // A run of fewer than one brace per dollar is content, which is how a raw + // interpolated string carries a literal brace + var braces = BraceRun(source, search); + if (braces < dollars) + { + search += braces; + continue; + } + + if (!TrySkipHole(source, ref search)) + { + index = source.Length; + return true; + } + + continue; + } + + if (ch != '"') { search++; continue; @@ -362,6 +386,18 @@ static bool TrySkipHole(string source, ref int cursor) return false; } + static int BraceRun(string source, int index) + { + var count = 0; + while (index + count < source.Length && + source[index + count] == '{') + { + count++; + } + + return count; + } + static int QuoteRun(string source, int index) { var count = 0; diff --git a/src/DiffEngine/Inline/CsStringLiteral.cs b/src/DiffEngine/Inline/CsStringLiteral.cs index 6f892a7c..58490598 100644 --- a/src/DiffEngine/Inline/CsStringLiteral.cs +++ b/src/DiffEngine/Inline/CsStringLiteral.cs @@ -14,7 +14,8 @@ public static class CsStringLiteral /// /// Renders (\n newlines) as a C# string literal expression: a /// regular literal when it is a single line, since a raw string spends three lines and an - /// indentation rule to say the same thing, and a multi-line raw literal otherwise. + /// indentation rule to say the same thing, and a multi-line raw literal otherwise - except + /// where the raw form cannot hold the content at all, which answers. /// /// Snapshot text with \n newlines. /// Whitespace prefix for content lines and the closing delimiter. @@ -42,8 +43,17 @@ public static string RenderRaw(string content, string indent, string eol) return "\"\""; } + if (StringLiteral.HasLineTerminator(content)) + { + // No delimiter can hold one of these, however wide: the compiler reads it as a line + // break, and the line it starts does not carry the closing delimiter's indentation + // (CS8999). Only a regular literal can say it, as an escape + return StringLiteral.RenderRegular(content); + } + // Three quotes, or one more than the longest run in the content, which is the widening - // F# does not have and the reason it needs a fallback where C# does not + // F# does not have and the reason a quote run sends it to a regular literal where C# stays + // raw var delimiter = new string('"', Math.Max(3, StringLiteral.LongestQuoteRun(content) + 1)); return StringLiteral.RenderMultiLine(content, indent, eol, delimiter); } @@ -107,20 +117,19 @@ static bool TryScanLiteral(string text, int start, out string? value, out int en return false; } - var quotes = StringLiteral.QuoteRunLength(text, index); - if (quotes >= 3) + if (verbatim) { - if (verbatim) - { - return false; - } - - return StringLiteral.TryScanMultiLine(text, index, quotes, out value, out end); + // Asked before the quote run is measured, because there is no verbatim raw form: a + // run of quotes after @" is an escaped quote and the start of the content, not a + // delimiter. Measuring first read @"""x""" as a raw string that happened to carry an + // @, and rejected a literal C# is perfectly happy with + return StringLiteral.TryScanVerbatim(text, index + 1, out value, out end); } - if (verbatim) + var quotes = StringLiteral.QuoteRunLength(text, index); + if (quotes >= 3) { - return StringLiteral.TryScanVerbatim(text, index + 1, out value, out end); + return StringLiteral.TryScanMultiLine(text, index, quotes, out value, out end); } if (quotes == 2) @@ -231,7 +240,7 @@ static bool TryScanRegular(string text, int start, out string? value, out int en return false; } - if (codePoint > 0x10FFFF) + if (!StringLiteral.IsScalarValue(codePoint)) { return false; } diff --git a/src/DiffEngine/Inline/FsStringLiteral.cs b/src/DiffEngine/Inline/FsStringLiteral.cs index 8ede64d1..20202261 100644 --- a/src/DiffEngine/Inline/FsStringLiteral.cs +++ b/src/DiffEngine/Inline/FsStringLiteral.cs @@ -38,9 +38,10 @@ public static string Render(string content, string indent, string eol) if (!CanTripleQuote(content)) { - // F# cannot widen a delimiter the way C# can (FS1232), so content that runs into one - // has no multi-line form at all. A regular literal on one source line always works, - // whatever it costs in escapes + // Content the multi-line form cannot hold: a quote run, which F# cannot widen a + // delimiter past the way C# can (FS1232), or a line terminator, which no delimiter + // helps with. A regular literal on one source line always works, whatever it costs in + // escapes return StringLiteral.RenderRegular(SourceLanguage.NormalizeNewlines(content)); } @@ -51,11 +52,19 @@ public static string Render(string content, string indent, string eol) /// Whether a triple-quoted literal can hold this content. A quote at either end would sit /// against the delimiter and be read as part of it, and a run of three anywhere would close /// the literal early. + /// + /// A line terminator rules it out as well, for the reason + /// gives. F# is stricter here than it has to be: + /// what sends C# to a regular literal is its own lexer reading one as a line break, and + /// nothing says F# does. It keeps the two languages writing the one shape, which is what + /// everything downstream of a render is written against. + /// /// static bool CanTripleQuote(string content) => content[0] != '"' && content[content.Length - 1] != '"' && - content.IndexOf("\"\"\"", StringComparison.Ordinal) == -1; + content.IndexOf("\"\"\"", StringComparison.Ordinal) == -1 && + !StringLiteral.HasLineTerminator(content); /// /// The snapshot a triple-quoted literal was written to hold: the value F# produced for it, @@ -245,7 +254,7 @@ static bool TryScanRegular(string text, int start, out string? value, out int en return false; } - if (codePoint > 0x10FFFF) + if (!StringLiteral.IsScalarValue(codePoint)) { return false; } diff --git a/src/DiffEngine/Inline/InlineApplier.cs b/src/DiffEngine/Inline/InlineApplier.cs index 9b2dd380..12d1e6a2 100644 --- a/src/DiffEngine/Inline/InlineApplier.cs +++ b/src/DiffEngine/Inline/InlineApplier.cs @@ -35,11 +35,6 @@ public static InlineApplyResult Apply(InlinePatch patch) return InlineApplyResult.Failed($"Invalid InlinePatch.SourceFile: {patch.SourceFile}", exception); } - if (!File.Exists(fullPath)) - { - return InlineApplyResult.Failed($"Source file does not exist: {fullPath}"); - } - var newContent = SourceLanguage.NormalizeNewlines(patch.NewContent); var normalizedPath = fullPath.ToLowerInvariant(); lock (gates.GetOrAdd(normalizedPath, static _ => new())) @@ -76,6 +71,15 @@ public static InlineApplyResult Apply(InlinePatch patch) static InlineApplyResult LockedApply(string fullPath, InlinePatch patch, string newContent) { + // Asked here rather than before the lock, because the swap at the end of this method takes + // the path away for the instant it takes to rename over it. Asked outside, an applier + // waiting its turn on a file another one was finishing with saw the file as missing and + // reported it, which is neither true nor the sort of thing a retry was going to fix + if (!File.Exists(fullPath)) + { + return InlineApplyResult.Failed($"Source file does not exist: {fullPath}"); + } + byte[] bytes; try { @@ -139,7 +143,7 @@ static InlineApplyResult LockedApply(string fullPath, InlinePatch patch, string output = content; } - File.WriteAllBytes(fullPath, output); + WriteThroughTemporary(fullPath, output); } catch (Exception exception) { @@ -149,6 +153,53 @@ static InlineApplyResult LockedApply(string fullPath, InlinePatch patch, string return InlineApplyResult.Applied; } + /// + /// Writes the patched source through a temporary file beside it and swaps that in, so the file + /// on disk is either what it was or what the patch made it and never half of either. + /// + /// Writing in place truncates the file first and fills it back in, which leaves a window where + /// a killed process or a full disk costs the caller the rest of their source file. The mutex + /// above keeps two appliers apart but says nothing about a process that stops partway. Every + /// other care taken here - strict decoders, the BOM round trip, refusing a file that will not + /// decode - is because the whole file is rewritten rather than the patched span, and the write + /// itself was the step that could still lose it. + /// + /// + /// The temporary is a sibling so the swap stays on one volume, where it is a rename rather + /// than a copy. Replace rather than a move that overwrites, because it keeps the attributes + /// the destination already had, and because the overwriting move does not exist on every + /// framework this targets. + /// + /// + static void WriteThroughTemporary(string fullPath, byte[] output) + { + var directory = Path.GetDirectoryName(fullPath)!; + // Named after the file it replaces, so anything left by a process that died between the + // write and the swap says what it was for. The extension keeps it out of a *.cs glob + var temporary = Path.Combine(directory, $"{Path.GetFileName(fullPath)}.{Guid.NewGuid():N}.tmp"); + try + { + File.WriteAllBytes(temporary, output); + File.Replace(temporary, fullPath, null); + } + finally + { + // Replace consumed it. Anything still there is this method's litter, and failing an + // applied patch over a temporary file that could not be deleted helps nobody + try + { + if (File.Exists(temporary)) + { + File.Delete(temporary); + } + } + catch (Exception exception) + when (exception is IOException or UnauthorizedAccessException) + { + } + } + } + /// /// The encoding to read and write the file with. Every one of them throws rather than /// substituting: the applier rewrites the whole file, not just the patched span, so a diff --git a/src/DiffEngine/Inline/InlineKey.cs b/src/DiffEngine/Inline/InlineKey.cs index 420f8fa6..465d3e5c 100644 --- a/src/DiffEngine/Inline/InlineKey.cs +++ b/src/DiffEngine/Inline/InlineKey.cs @@ -12,9 +12,19 @@ namespace DiffEngine; public static class InlineKey { /// - /// Case folded, because Windows paths reach here from different sources with different casing - /// and the same call site must produce one entry. + /// Case folded where the file system is, because a Windows path reaches here from different + /// sources with different casing and the same call site has to produce one entry. + /// + /// Not where it is not. On Linux two paths differing only in case are two files, and folding + /// them gave both one key: the second patch took over the first's entry, and settling either + /// settled both. Every process addressing a queue is on the one machine, so they agree about + /// which of these applies. + /// /// public static string For(string sourceFile, int line) => - $"{sourceFile.ToLowerInvariant()}|{line}"; + $"{(caseInsensitivePaths ? sourceFile.ToLowerInvariant() : sourceFile)}|{line}"; + + static readonly bool caseInsensitivePaths = + RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || + RuntimeInformation.IsOSPlatform(OSPlatform.OSX); } diff --git a/src/DiffEngine/Inline/InlinePatchFile.cs b/src/DiffEngine/Inline/InlinePatchFile.cs index b9f2bc30..58ddc4e8 100644 --- a/src/DiffEngine/Inline/InlinePatchFile.cs +++ b/src/DiffEngine/Inline/InlinePatchFile.cs @@ -19,6 +19,14 @@ public static void Write(string path, InlinePatch patch) public static string Build(InlinePatch patch) { + // The two fields that ride the payload as themselves rather than base64, so a line break + // in either would end its line and the rest would be read as more of the payload - + // shifting the fixed lines, or forging one of the trailing ones. Both are public settable + // properties, and a path may legally hold a line break off Windows. Refused rather than + // written, because there is no shape of this format that can carry one + AgainstLineBreak(patch.SourceFile, nameof(InlinePatch.SourceFile)); + AgainstLineBreak(patch.Framework, nameof(InlinePatch.Framework)); + var expression = patch.OriginalExpression is null ? "" : Convert.ToBase64String(Encoding.UTF8.GetBytes(patch.OriginalExpression)); @@ -78,7 +86,11 @@ public static bool TryParse(string text, [NotNullWhen(true)] out InlinePatch? pa !TryValue(lines[2], "lineHint", out var lineText) || !int.TryParse(lineText, out var lineHint) || !TryValue(lines[3], "mode", out var modeText) || + // IsDefined as well, because TryParse takes a number as readily as a name: "mode: 7" + // parsed to an InlinePatchMode that is none of them, and then fell through every mode + // check in the patcher to behave as a Set !Enum.TryParse(modeText, out var mode) || + !Enum.IsDefined(typeof(InlinePatchMode), mode) || !TryValue(lines[4], "originalExpression", out var expressionBase64) || !TryValue(lines[5], "newContent", out var contentBase64)) { @@ -147,6 +159,15 @@ public static bool TryParse(string text, [NotNullWhen(true)] out InlinePatch? pa return true; } + static void AgainstLineBreak(string? value, string name) + { + if (value is not null && + (value.IndexOf('\n') != -1 || value.IndexOf('\r') != -1)) + { + throw new ArgumentException($"InlinePatch.{name} cannot contain a line break. Value: {value}"); + } + } + static bool TryValue(string line, string key, out string value) { value = ""; diff --git a/src/DiffEngine/Inline/InlineQueue.cs b/src/DiffEngine/Inline/InlineQueue.cs index 496f6a2e..be5bf755 100644 --- a/src/DiffEngine/Inline/InlineQueue.cs +++ b/src/DiffEngine/Inline/InlineQueue.cs @@ -58,6 +58,15 @@ public InlineQueue Enqueue(InlinePatch patch) return new(items); } + /// + /// A fold that added nothing. The variants are handed back as they are, so an accept applying + /// against this entry still recognises it as the one it started on; only the status of the + /// last attempt goes, which is what rebuilding the entry used to do anyway - the content + /// arrived again, and what failed before has not been retried. + /// + static PendingInline Unchanged(PendingInline entry) => + entry.Status is null ? entry : entry with { Status = null }; + static PendingInline Fold(PendingInline entry, InlinePatch patch) { var origin = patch.Framework; @@ -67,6 +76,17 @@ static PendingInline Fold(PendingInline entry, InlinePatch patch) if (origin is null || entry.Variants.All(_ => _.Origins.Count == 0)) { + // A re-run that repeats itself has changed nothing, and saying so matters: Accept + // throws away the completion of an accept whose entry changed identity while the + // patch was applying, and a still failing test re-sending the same patch is exactly + // what happens during those ten seconds + if (entry.Variants is [var only] && + only.Origins.Count == 0 && + only.Patch.Matches(patch)) + { + return Unchanged(entry); + } + return new(patch); } @@ -79,6 +99,9 @@ static PendingInline Fold(PendingInline entry, InlinePatch patch) if (matching >= 0) { var folded = new List(); + // Nothing to say when this framework already held this content and no other variant + // gave a label up, which is the shape a re-run repeating itself arrives in + var changed = false; for (var index = 0; index < variants.Count; index++) { var variant = variants[index]; @@ -86,24 +109,38 @@ static PendingInline Fold(PendingInline entry, InlinePatch patch) { // The existing patch instance, deliberately: the content is identical, and a // reader caching per patch keeps its work. - folded.Add(variant.Origins.Contains(origin) - ? variant - : variant with { Origins = [.. variant.Origins, origin] }); + if (variant.Origins.Contains(origin)) + { + folded.Add(variant); + } + else + { + folded.Add(variant with { Origins = [.. variant.Origins, origin] }); + changed = true; + } + continue; } var stripped = variant.Origins.Where(_ => _ != origin).ToList(); if (stripped.Count == 0) { + changed = true; continue; } - folded.Add(stripped.Count == variant.Origins.Count - ? variant - : variant with { Origins = stripped }); + if (stripped.Count == variant.Origins.Count) + { + folded.Add(variant); + } + else + { + folded.Add(variant with { Origins = stripped }); + changed = true; + } } - return new(folded); + return changed ? new(folded) : Unchanged(entry); } // This framework previously produced different content: its variant updates in place when diff --git a/src/DiffEngine/Inline/InlineQueueClient.cs b/src/DiffEngine/Inline/InlineQueueClient.cs index 3068094a..aaac0098 100644 --- a/src/DiffEngine/Inline/InlineQueueClient.cs +++ b/src/DiffEngine/Inline/InlineQueueClient.cs @@ -6,8 +6,14 @@ namespace DiffEngine; public enum InlineAcceptOutcome { /// - /// Nothing happened: no owner answered, or the one that did holds no entry for that key β€” - /// it settled, or another surface got to it first. + /// There is nothing to report. No owner answered, or the one that did holds no entry for that + /// key β€” it settled, or another surface got to it first. + /// + /// Also the answer when an owner took the accept and then could not be asked what became of + /// it. That is rare and it is not nothing, but the two outcomes it sits between are worse + /// guesses: stops a caller offering a snapshot that may still be + /// pending, and invites a retry of one that is probably already applied. + /// /// Unknown, @@ -74,7 +80,10 @@ public static class InlineQueueClient /// public static bool TryList(out IReadOnlyList pending) { - if (!Exchange(new(ViewerVerb.ListFull), ViewerClient.ShortTimeout, out var response)) + // An owner that answered with an error has not told us what it holds, and an empty item + // list on an error is not the same statement as an empty queue + if (!Exchange(new(ViewerVerb.ListFull), ViewerClient.ShortTimeout, out var response) || + !response.Ok) { pending = []; return false; @@ -95,7 +104,8 @@ public static bool TryList(out IReadOnlyList pending) /// public static bool TryListKeys(out IReadOnlyList keys) { - if (!Exchange(new(ViewerVerb.List), ViewerClient.ShortTimeout, out var response)) + if (!Exchange(new(ViewerVerb.List), ViewerClient.ShortTimeout, out var response) || + !response.Ok) { keys = []; return false; @@ -117,8 +127,8 @@ public static bool TryListKeys(out IReadOnlyList keys) /// /// Asks the owner to apply the patch for a call site and drop it from the queue. /// is the owner's own account of what happened, suitable to show a - /// user as it stands, and null when it had nothing to say β€” always so for - /// , where nothing happened. + /// user as it stands, and null when it had nothing to say β€” which is most of the time for + /// , since nothing usually happened. /// public static InlineAcceptOutcome Accept(string key, out string? message) { @@ -133,7 +143,7 @@ public static InlineAcceptOutcome Accept(string key, out string? message) { // One error shape covers both "no entry for that key" and a refusal on a live one β€” a // conflicted entry β€” so which it was is asked rather than read out of the text. - if (StillPending(key)) + if (StillPending(key) == true) { return InlineAcceptOutcome.Failed; } @@ -147,9 +157,15 @@ public static InlineAcceptOutcome Accept(string key, out string? message) // Attempted, but attempted is not applied: an owner keeps an entry that failed to write so // it can be retried. Whether the entry survived is the answer, and it has to be asked for // rather than read off `ok`. - return StillPending(key) - ? InlineAcceptOutcome.Failed - : InlineAcceptOutcome.Accepted; + return StillPending(key) switch + { + true => InlineAcceptOutcome.Failed, + false => InlineAcceptOutcome.Accepted, + // The owner took the accept and then could not be asked what became of it. Reading + // that as applied is a guess, and the one that loses a snapshot: a caller told it was + // accepted stops offering it + null => InlineAcceptOutcome.Unknown + }; } /// @@ -177,9 +193,15 @@ public static bool Focus(string key) => Exchange(new(ViewerVerb.Focus, key), ViewerClient.ShortTimeout, out var response) && response.Ok; - static bool StillPending(string key) => - TryListKeys(out var keys) && - keys.Contains(key); + /// + /// Whether the owner still holds the entry, or null when it could not be asked - it went away, + /// or answered with an error. Three answers rather than two because "it is not pending" and + /// "there is no telling" lead to opposite reports, and a failed listing used to give the first. + /// + static bool? StillPending(string key) => + TryListKeys(out var keys) + ? keys.Contains(key) + : null; static string? Text(string? message) => message is { Length: > 0 } ? message : null; diff --git a/src/DiffEngine/Inline/SolutionDirectoryFinder.cs b/src/DiffEngine/Inline/SolutionDirectoryFinder.cs index 4cb9b04e..127098af 100644 --- a/src/DiffEngine/Inline/SolutionDirectoryFinder.cs +++ b/src/DiffEngine/Inline/SolutionDirectoryFinder.cs @@ -6,7 +6,13 @@ class Result(string directory, string name) public string Name { get; } = name; } - static ConcurrentDictionary cache = new(StringComparer.OrdinalIgnoreCase); + static readonly ConcurrentDictionary cache = new(StringComparer.OrdinalIgnoreCase); + + /// + /// Directories already known to hold a solution, which is what lets the walk stop early + /// without asking the disk again. + /// + static readonly ConcurrentDictionary directories = new(StringComparer.OrdinalIgnoreCase); /// /// The solution a file belongs to, or null when it has none. @@ -37,29 +43,6 @@ class Result(string directory, string name) static Result? Walk(string file) { - // 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 (result == null || - !IsInDirectory(file, result.Directory)) - { - 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)) { @@ -68,6 +51,16 @@ class Result(string directory, string name) do { + // Asked level by level on the way up, so the nearest solution is always the one that + // answers. Reuse used to be a scan for any cached directory the file sat under, which + // took whichever had been resolved first: a file in a nested solution was given the + // one above it, because that directory encloses it too and nothing had yet looked + // between the two + if (directories.TryGetValue(currentDirectory, out var known)) + { + return known; + } + if (TryFind(currentDirectory, "*.slnx", out var result)) { return result; @@ -88,25 +81,6 @@ 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) { string[] solutions; @@ -127,6 +101,7 @@ static bool TryFind(string directory, string searchPattern, [NotNullWhen(true)] if (solutions.Length != 0) { result = new(directory, Path.GetFileNameWithoutExtension(solutions.First())); + directories[directory] = result; return true; } diff --git a/src/DiffEngine/Inline/StringLiteral.cs b/src/DiffEngine/Inline/StringLiteral.cs index 6070ec38..040a2984 100644 --- a/src/DiffEngine/Inline/StringLiteral.cs +++ b/src/DiffEngine/Inline/StringLiteral.cs @@ -59,8 +59,12 @@ public static string RenderRegular(string content) continue; } - // Everything else a literal cannot carry as itself - if (ch < ' ' || ch == '\u007f') + // Everything else a literal cannot carry as itself. Past the C0 range that is the + // three line terminators recognised beyond \n and \r: left as themselves they end the + // literal rather than sit in it + if (ch < ' ' || + ch == '\u007f' || + IsLineTerminator(ch)) { builder.Append("\\u"); builder.Append(((int) ch).ToString("x4")); @@ -74,6 +78,37 @@ public static string RenderRegular(string content) return builder.ToString(); } + /// + /// The line terminators a lexer recognises beyond \n and \r: next line, line + /// separator and paragraph separator. + /// + /// A C# literal cannot hold one as itself. In a regular literal it ends the literal (CS1010), + /// and in a raw one it reads as a line break, so the line after it no longer starts with the + /// whitespace the closing delimiter defines (CS8999). Only an escape carries one, and only a + /// regular literal has escapes, which is why content holding one is written as a regular + /// literal whatever else it holds. + /// + /// + public static bool IsLineTerminator(char ch) => + ch is (char) 0x85 or (char) 0x2028 or (char) 0x2029; + + /// + /// True when the content holds a terminator describes, which is + /// what sends content that would otherwise be written multi-line to a regular literal instead. + /// + public static bool HasLineTerminator(string content) + { + foreach (var ch in content) + { + if (IsLineTerminator(ch)) + { + return true; + } + } + + return false; + } + /// /// Renders (\n newlines) as a multi-line literal delimited by /// . The result starts with the opening delimiter (no leading @@ -285,6 +320,20 @@ public static bool TryScanVerbatim(string text, int start, out string? value, ou return false; } + /// + /// Whether a code point is one a string can hold: inside the Unicode range, and not half of a + /// surrogate pair written on its own. + /// + /// Asked ahead of , which throws on either. A parse + /// that throws is not a parse that failed: callers handle false, and between here and the + /// process hosting the queue nothing catches. Source holding one of these does not compile, so + /// the answer being looked for is that this is not a literal that can be read. + /// + /// + public static bool IsScalarValue(uint codePoint) => + codePoint <= 0x10FFFF && + codePoint is < 0xD800 or > 0xDFFF; + public static bool TryReadHex(string text, ref int index, int min, int max, out uint result) { result = 0; diff --git a/src/DiffEngineTray.Tests/FakeViewer.cs b/src/DiffEngineTray.Tests/FakeViewer.cs index 883ef4b0..0a480ab1 100644 --- a/src/DiffEngineTray.Tests/FakeViewer.cs +++ b/src/DiffEngineTray.Tests/FakeViewer.cs @@ -39,6 +39,12 @@ public FakeViewer(params string[] names) public string? FailureMessage { get; set; } = "the file is locked"; + /// + /// When true, listings come back as an error, which stands in for an owner that took a verb + /// and then could not be asked what it holds. + /// + public bool ListingFails { get; set; } + async Task Listen() { while (!cancel.IsCancellationRequested) @@ -68,6 +74,14 @@ string Respond(string request) Verbs.Add(key == null ? verb : $"{verb}:{key}"); var builder = new StringBuilder("version: 1\n"); + if (verb == "list" && + ListingFails) + { + builder.Append("status: error\n"); + Append(builder, "message", "the owner is going away"); + return builder.ToString(); + } + if (verb is "accept" or "discard" or "acceptall" or "discardall" && !Succeed) { diff --git a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs index 94305cff..5cb58fd8 100644 --- a/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs +++ b/src/DiffEngineTray.Tests/OwnedInlineHostTest.cs @@ -318,13 +318,15 @@ public async Task AReplacedEntrySurvivesTheAcceptItInterrupted() }); try { - owner.Queue(); + owner.Queue(content: "first"); var snapshot = owner.Host.List().Single(); var accepting = Task.Run(() => owner.Send(new(ViewerVerb.Accept, snapshot.Key))); applying.Wait(TimeSpan.FromSeconds(5)); - // The same call site again, mid apply, as a re-run of the test would send it. - owner.Queue(); + // The same call site again, mid apply, saying something else: a re-run of a test + // whose result moved on. What was applied is the old content, so the outcome + // describes nothing that is pending now + owner.Queue(content: "second"); release.Set(); var accepted = await accepting; @@ -337,6 +339,43 @@ public async Task AReplacedEntrySurvivesTheAcceptItInterrupted() } } + /// + /// The same window, and the far more common arrival in it: a still failing test re-running and + /// producing what it produced before. Nothing was replaced, so the accept completes and the + /// entry goes - rather than the patch reaching the file while the entry stays pending with + /// nothing said about it. + /// + [Test] + public async Task AnIdenticalReRunDoesNotInterruptTheAcceptItLandsIn() + { + using var applying = new ManualResetEventSlim(); + using var release = new ManualResetEventSlim(); + using var owner = new Owner(_ => + { + applying.Set(); + release.Wait(TimeSpan.FromSeconds(10)); + return InlineApplyResult.Applied; + }); + try + { + owner.Queue(content: "same"); + var snapshot = owner.Host.List().Single(); + var accepting = Task.Run(() => owner.Send(new(ViewerVerb.Accept, snapshot.Key))); + applying.Wait(TimeSpan.FromSeconds(5)); + + owner.Queue(content: "same"); + release.Set(); + var accepted = await accepting; + + await Assert.That(accepted.Ok).IsTrue(); + await Assert.That(owner.Host.List()).IsEmpty(); + } + finally + { + release.Set(); + } + } + /// /// The ownership gate. A tray that starts while a viewer holds the port does not own the /// queue, and drives it remotely for the rest of its life instead. diff --git a/src/DiffEngineTray.Tests/PiperTest.cs b/src/DiffEngineTray.Tests/PiperTest.cs index 7a64104b..32e17ec2 100644 --- a/src/DiffEngineTray.Tests/PiperTest.cs +++ b/src/DiffEngineTray.Tests/PiperTest.cs @@ -130,6 +130,32 @@ public async Task ClientDisconnectsAbruptly() await Assert.That(received!.File).IsEqualTo("Foo"); } + /// + /// A client that connects and never finishes sending β€” a test process wedged mid write β€” used + /// to hold the one accept loop for as long as it stayed that way, so every move and delete + /// from every other process on the machine went nowhere and nothing timed the wait out. + /// + [Test] + public async Task AClientThatStopsSendingDoesNotBlockTheNextOne() + { + DeletePayload? received = null; + using var source = new CancelSource(); + var task = PiperServer.Start(_ => { }, _ => received = _, source.Token); + + // Connected, wrote nothing, and holds the stream open for the rest of the test + using var wedged = new TcpClient(); + await wedged.ConnectAsync(IPAddress.Loopback, PiperClient.Port, source.Token); + await using var held = wedged.GetStream(); + + await PiperClient.SendDeleteAsync("Foo", source.Token); + await Task.Delay(1000, source.Token); + await source.CancelAsync(); + await task; + + await Assert.That(received).IsNotNull(); + await Assert.That(received!.File).IsEqualTo("Foo"); + } + [Test] public async Task SendOnly() { diff --git a/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs b/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs index 7f3d271f..37473d24 100644 --- a/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs +++ b/src/DiffEngineTray.Tests/SolutionDirectoryFinderTests.cs @@ -42,6 +42,35 @@ public async Task MissingDirectoryIsWalkedPast() public async Task UnusablePathIsNotFound() => await Assert.That(SolutionDirectoryFinder.Find("no\0such\\Sample.verified.txt")).IsNull(); + /// + /// A solution inside another one owns the files under it. Reuse used to be a scan of every + /// resolved directory for one the file sat under, so once the outer solution was cached it + /// answered for the inner one too β€” the outer directory encloses those files, and nothing had + /// looked between the file and it. Whichever was resolved first decided, which made the bug + /// depend on the order patches happened to arrive in. + /// + [Test] + public async Task NestedSolutionOwnsItsOwnFiles() + { + var root = Path.Combine(Path.GetTempPath(), "DiffEngineSlnFinder", Guid.NewGuid().ToString("N")); + var nested = Path.Combine(root, "submodule", "Tests"); + Directory.CreateDirectory(nested); + try + { + await File.WriteAllTextAsync(Path.Combine(root, "Outer.sln"), ""); + await File.WriteAllTextAsync(Path.Combine(root, "submodule", "Inner.slnx"), ""); + + // The outer one first, so its directory is the cached one + await Assert.That(SolutionDirectoryFinder.Find(Path.Combine(root, "Class.cs"))).IsEqualTo("Outer"); + + await Assert.That(SolutionDirectoryFinder.Find(Path.Combine(nested, "Tests.cs"))).IsEqualTo("Inner"); + } + finally + { + Directory.Delete(root, true); + } + } + [Test] public async Task SiblingWithSharedPrefixIsNotMatched() { diff --git a/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs b/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs index beaaea7a..ebab44ec 100644 --- a/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs +++ b/src/DiffEngineTray.Tests/TrackerSnapshotTest.cs @@ -40,6 +40,26 @@ public async Task AcceptForwardsTheKeyAndRefreshes() await Assert.That(tracker.Snapshots[0].Name).IsEqualTo("Other.cs:1"); } + /// + /// Whether the entry survived is what tells an accept from a failure here, and that is a + /// second round trip. An owner that took the accept and then could not answer used to come + /// back as applied, because a listing that failed returned no items and no items read as an + /// empty queue. + /// + [Test] + public async Task AnAcceptThatCannotBeConfirmedIsNotCalledApplied() + { + using var viewer = new FakeViewer("Sample.cs:1"); + var host = new RemoteInlineHost(); + var snapshot = host.List().Single(); + viewer.ListingFails = true; + + var outcome = host.Accept(snapshot, out _); + + await Assert.That(outcome).IsEqualTo(AcceptOutcome.Unknown); + await Assert.That(viewer.Verbs).Contains($"accept:{snapshot.Key}"); + } + [Test] public async Task DiscardForwardsTheKey() { diff --git a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs index b1b4977e..644643cd 100644 --- a/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs +++ b/src/DiffEngineTray.Tests/TrayViewerSyncTest.cs @@ -251,6 +251,28 @@ public async Task ViewerAcceptOfATrackedMoveReachesTheTray() await Assert.That(await File.ReadAllTextAsync(move.Target)).IsEqualTo("received"); } + /// + /// The tray menu is built from the last scan, so an item can outlive its entry: the test + /// re-ran and passed, or the viewer accepted it first. Clicking it accepts nothing, and there + /// is nothing to tell the user β€” a failure balloon there names a snapshot that is already in + /// the source. Accepting a group has always skipped these; accepting one of them said the + /// accept had failed. + /// + [Test] + public async Task AcceptingASnapshotThatIsAlreadyGoneSaysNothing() + { + await using var pair = new TrayOwned(); + var snapshot = pair.Queue(sample, 1); + pair.Queue(other, 7); + pair.Tracker.Discard(snapshot); + + await pair.Tracker.Accept(snapshot); + + await Assert.That(pair.Failures).IsEmpty(); + await Assert.That(pair.Applied).IsEmpty(); + await Assert.That(pair.Tracker.Snapshots.Select(_ => _.Key)).IsEquivalentTo([Key(other, 7)]); + } + /// /// A failed apply keeps its entry so it can be retried, and both surfaces have to say the same /// thing about it β€” the tray in a balloon, the viewer in the entry's status. @@ -339,6 +361,24 @@ public async Task TrayAcceptAllEmptiesTheOwningViewer() await Assert.That(pair.Applied.Count).IsEqualTo(2); } + /// + /// A tray owner asks for the window on every patch that arrives, and an owning viewer has to + /// do the same. Its window is hidden whenever a tray is running and the queue last emptied, so + /// without this a newly failing test landed in a window nobody could see and showed up only as + /// a tray icon on the next scan. + /// + [Test] + public async Task AQueuedPatchBringsTheOwningViewerForward() + { + await using var pair = new ViewerOwned(); + + pair.Queue(sample, 1); + + await Assert.That(pair.Windows).Contains(ViewerSideWindowCommand.Focus); + // On the entry that arrived, the same as a focus verb lands + await Assert.That(pair.Viewer.Queue[pair.Viewer.Selected].Key).IsEqualTo(Key(sample, 1)); + } + [Test] public async Task TrayAcceptOfOneSnapshotLeavesTheRestInTheOwningViewer() { @@ -456,6 +496,72 @@ public async Task TrayAcceptAllReportsWhatTheOwningViewerKept() await Assert.That(pair.Failures.Single()).Contains("the file is locked"); } + /// + /// An owning viewer applies inside its session, and InlineApplier waits up to ten seconds on + /// its cross process mutex, so an accept legitimately outlasts the wait the listing verbs use. + /// The tray has to wait for the answer: reporting the viewer as gone while it is in the middle + /// of writing the source file is wrong twice over, since the snapshot then leaves the menu a + /// scan later and contradicts the balloon. + /// + [Test] + public async Task ASlowAcceptIsWaitedForRatherThanCalledAMissingViewer() + { + await using var pair = new ViewerOwned( + _ => + { + Thread.Sleep(TimeSpan.FromSeconds(1.5)); + return ViewerSideApplyResult.Applied; + }); + var snapshot = pair.Snapshot(sample, 1); + + await pair.Tracker.Accept(snapshot); + + await Assert.That(pair.Failures).IsEmpty(); + await Assert.That(pair.Viewer.Queue).IsEmpty(); + await Assert.That(pair.Tracker.Snapshots).IsEmpty(); + } + + /// + [Test] + public async Task ASlowAcceptAllIsWaitedFor() + { + // Every entry applied inside the one exchange, so this outlasts a lone accept + await using var pair = new ViewerOwned( + _ => + { + Thread.Sleep(TimeSpan.FromSeconds(0.8)); + return ViewerSideApplyResult.Applied; + }); + pair.Queue(sample, 1); + pair.Queue(other, 7); + + await pair.Tracker.AcceptAll(); + + await Assert.That(pair.Failures).IsEmpty(); + await Assert.That(pair.Viewer.Queue).IsEmpty(); + await Assert.That(pair.Applied.Count).IsEqualTo(2); + } + + /// + /// "Close snapshot viewer" closes a window. Against an owning viewer it used to quit the + /// process, and the queue is in that process's memory, so the same menu item threw away every + /// pending snapshot without asking - and they were simply absent from the menu afterwards, + /// since a refused connection lists the same as nothing pending. + /// + [Test] + public async Task ClosingAnOwningViewerLeavesItsQueue() + { + await using var pair = new ViewerOwned(); + pair.Queue(sample, 1); + pair.Queue(other, 7); + + new RemoteInlineHost().Close(); + + await Assert.That(pair.Windows).Contains(ViewerSideWindowCommand.Hide); + await Assert.That(pair.Windows).DoesNotContain(ViewerSideWindowCommand.Close); + await Assert.That(pair.Viewer.Keys()).IsEquivalentTo([Key(sample, 1), Key(other, 7)]); + } + [Test] public async Task AConflictIsRefusedTheSameWayFromTheTray() { diff --git a/src/DiffEngineTray/IInlineHost.cs b/src/DiffEngineTray/IInlineHost.cs index 5e64d665..8ceec101 100644 --- a/src/DiffEngineTray/IInlineHost.cs +++ b/src/DiffEngineTray/IInlineHost.cs @@ -34,8 +34,9 @@ interface IInlineHost void Focus(PendingSnapshot snapshot); /// - /// Close the window. The queue is unaffected either way: an owning viewer exits with it, and a - /// tray owned queue simply loses its display until something reopens one. + /// Close the window. The queue is unaffected either way: a tray owned queue loses its display + /// until something reopens one, and an owning viewer is asked to hide rather than to quit, + /// since quitting would take the queue it holds with it. /// void Close(); } diff --git a/src/DiffEngineTray/IViewerLauncher.cs b/src/DiffEngineTray/IViewerLauncher.cs index 487c92de..db802332 100644 --- a/src/DiffEngineTray/IViewerLauncher.cs +++ b/src/DiffEngineTray/IViewerLauncher.cs @@ -22,7 +22,12 @@ sealed class ProcessViewerLauncher : IViewerLauncher public bool Launch() { + // The one it replaces has exited - Launch is only reached when Running said so - but the + // handle it was read through has not gone anywhere. The tray runs for weeks, and every + // relaunch over that time used to leave one behind for a finaliser to get to eventually + var previous = viewer; viewer = ViewerLauncher.LaunchAttached(); + previous?.Dispose(); return viewer is not null; } } diff --git a/src/DiffEngineTray/PiperServer.cs b/src/DiffEngineTray/PiperServer.cs index e751441e..4a77cae9 100644 --- a/src/DiffEngineTray/PiperServer.cs +++ b/src/DiffEngineTray/PiperServer.cs @@ -30,6 +30,9 @@ public static async Task Start( { listener = new(IPAddress.Loopback, PiperClient.Port); listener.Start(); + // Kept from when the accept lived inside the per-connection method: cancelling stops + // the listener, which is what brings a pending accept down with it + await using var registration = cancel.Register(listener.Stop); while (true) { @@ -40,7 +43,13 @@ public static async Task Start( try { - await Handle(listener, move, delete, cancel); + var client = await listener.AcceptTcpClientAsync(cancel); + // On its own task, the way ViewerServer takes its connections. Handled in + // turn, one client that connected and never closed its stream held up every + // move and delete from every other process for as long as it stayed that way, + // with nothing to end the wait. The callbacks are the tracker's concurrent + // collections, which the viewer port already writes to off this thread + _ = Handle(client, move, delete, cancel); } catch (TaskCanceledException) { @@ -73,39 +82,82 @@ public static async Task Start( } } - static async Task Handle(TcpListener listener, Action move, Action delete, Cancel cancel) + /// + /// How long one client gets to send its payload and close. A client is expected to write and + /// go, so anything near this is one that has stopped rather than one that is slow, and the + /// read has to end by itself: nothing else here will end it. + /// + static readonly TimeSpan readTimeout = TimeSpan.FromSeconds(10); + + /// + /// Nothing awaits this, so it reports rather than throws β€” an unobserved throw on the + /// finaliser thread is not a way to hear about a dropped move. + /// + static async Task Handle(TcpClient client, Action move, Action delete, Cancel cancel) { - await using (cancel.Register(listener.Stop)) + try { - using var client = await listener.AcceptTcpClientAsync(cancel); - using var reader = new StreamReader(client.GetStream()); - - var payload = await reader.ReadToEndAsync(cancel); - - if (payload.Contains("\"Type\":\"Move\"") || - payload.Contains("\"Type\": \"Move\"")) - { - move(Serializer.Deserialize(payload)); - } - else if (payload.Contains("\"Type\":\"Delete\"") || - payload.Contains("\"Type\": \"Delete\"")) - { - delete(Serializer.Deserialize(payload)); - } - else + using (client) { - if (payload.Length > 0) + using var reader = new StreamReader(client.GetStream()); + using var deadline = CancellationTokenSource.CreateLinkedTokenSource(cancel); + deadline.CancelAfter(readTimeout); + + string payload; + try { - // Tolerate payloads from newer clients so future additions dont - // surface an error dialog on this tray version - Log.Error("Received unknown payload type. Ignoring. Payload: {payload}", payload); + payload = await reader.ReadToEndAsync(deadline.Token); + } + catch (OperationCanceledException) + when (!cancel.IsCancellationRequested) + { + Log.Error("A client connected and did not finish sending within {timeout}. Ignoring it.", readTimeout); + return; } - } - if (client.Connected) + Dispatch(payload, move, delete); + } + } + catch (Exception exception) + when (exception is OperationCanceledException or ObjectDisposedException) + { + // Shutting down, or the socket went with it + } + catch (IOException exception) + when (exception.InnerException is SocketException {SocketErrorCode: SocketError.ConnectionReset}) + { + //client disconnected abruptly, e.g. test was canceled + } + catch (Exception exception) + { + if (!cancel.IsCancellationRequested) { - client.Close(); + ExceptionHandler.Handle("Failed to receive payload", exception); } } } + + static void Dispatch(string payload, Action move, Action delete) + { + if (payload.Contains("\"Type\":\"Move\"") || + payload.Contains("\"Type\": \"Move\"")) + { + move(Serializer.Deserialize(payload)); + return; + } + + if (payload.Contains("\"Type\":\"Delete\"") || + payload.Contains("\"Type\": \"Delete\"")) + { + delete(Serializer.Deserialize(payload)); + return; + } + + if (payload.Length > 0) + { + // Tolerate payloads from newer clients so future additions dont + // surface an error dialog on this tray version + Log.Error("Received unknown payload type. Ignoring. Payload: {payload}", payload); + } + } } \ No newline at end of file diff --git a/src/DiffEngineTray/Program.cs b/src/DiffEngineTray/Program.cs index 1bb2ab9c..c5684f14 100644 --- a/src/DiffEngineTray/Program.cs +++ b/src/DiffEngineTray/Program.cs @@ -90,7 +90,11 @@ void Warn(string message) => owned.Start(); } - using var task = StartServer(tracker, cancel); + // Not a using. Anything throwing between here and the await below would dispose a task + // that is still running, and Task.Dispose throws for one that has not completed - which + // would replace whatever actually went wrong with an InvalidOperationException. A task + // needs no disposal anyway; cancelling it is what ends it + var task = StartServer(tracker, cancel); using var keyRegister = new KeyRegister(icon.Handle()); ReBindKeys(settings, keyRegister, tracker); diff --git a/src/DiffEngineTray/RemoteInlineHost.cs b/src/DiffEngineTray/RemoteInlineHost.cs index 31197ddd..26397e5b 100644 --- a/src/DiffEngineTray/RemoteInlineHost.cs +++ b/src/DiffEngineTray/RemoteInlineHost.cs @@ -2,8 +2,12 @@ /// The queue belongs to a viewer that bound the port before this tray started, so every call is a /// short loopback round trip and the tray is a remote control. /// -/// All of them use ViewerClient.ShortTimeout. These run from the 2 second scan timer and from the -/// menu opening, so a slow exchange must not outlast the timer period or block the UI. +/// The listing and the menu verbs use ViewerClient.ShortTimeout. Those run from the 2 second scan +/// timer and from the menu opening, so a slow exchange must not outlast the timer period or block +/// the UI. +/// +/// +/// Accepting does not, for the reason gives. /// /// /// A refused connection means the viewer has gone, which is the same as nothing pending. The queue @@ -12,19 +16,44 @@ /// class RemoteInlineHost : IInlineHost { + /// + /// The one verb that can legitimately take this long: the owner applies through + /// , which waits up to ten seconds on its cross process mutex, and + /// an owning viewer does that inside its session. The short wait read a busy owner as an + /// absent one and told the user the viewer was not running while it was in the middle of + /// writing their source file - and then the snapshot left the menu a scan later, contradicting + /// the balloon. + /// + /// The same wait and OwnerLink use, and safe here for the same + /// reason it is there: accepts run on a worker rather than the timer or the UI thread, which + /// is what exists to arrange. + /// + /// + static readonly TimeSpan acceptWait = TimeSpan.FromSeconds(15); + public string Description => $"owned by another process on port {ViewerClient.Port}"; - public IReadOnlyList List() + public IReadOnlyList List() => + TryList(out var pending) ? pending : []; + + /// + /// False when the owner could not be asked, which flattens to nothing + /// pending β€” right for a menu, and wrong for anything reading the answer as a statement about + /// a particular entry. + /// + static bool TryList(out IReadOnlyList pending) { - if (!Exchange(new(ViewerVerb.List), out var response) || + if (!Exchange(new(ViewerVerb.List), ViewerClient.ShortTimeout, out var response) || !response.Ok) { - return []; + pending = []; + return false; } - return response.Items + pending = response.Items .Select(_ => new PendingSnapshot(_.Key, _.Name, _.Status)) .ToList(); + return true; } public IReadOnlyList? Queued() => @@ -50,41 +79,62 @@ public IReadOnlyList List() /// public AcceptOutcome Accept(PendingSnapshot snapshot, out string? message) { - if (!Send(ViewerVerb.Accept, snapshot.Key, out message)) + if (!Send(ViewerVerb.Accept, snapshot.Key, acceptWait, out message)) { return AcceptOutcome.Failed; } - return List().Any(_ => _.Key == snapshot.Key) + if (!TryList(out var pending)) + { + // The owner took the accept and then could not be asked what became of it. Applied is + // a guess, and the one that tells the user a snapshot landed that may not have + return AcceptOutcome.Unknown; + } + + return pending.Any(_ => _.Key == snapshot.Key) ? AcceptOutcome.Failed : AcceptOutcome.Applied; } public bool Discard(PendingSnapshot snapshot, out string? message) => - Send(ViewerVerb.Discard, snapshot.Key, out message); + Send(ViewerVerb.Discard, snapshot.Key, ViewerClient.ShortTimeout, out message); /// /// True only when the queue is empty afterwards, for the reason gives β€” /// and matching what an owning tray reports, which is also "is anything still pending". A /// conflict counts as not accepted, which is right: it is what a reviewer still has to resolve. /// + // One accept per entry inside a single exchange, so this outlasts a lone accept rather than + // matching it public bool AcceptAll(out string? message) => - Send(ViewerVerb.AcceptAll, null, out message) && + Send(ViewerVerb.AcceptAll, null, acceptWait, out message) && List().Count == 0; public void DiscardAll() => - Send(ViewerVerb.DiscardAll, null, out _); + Send(ViewerVerb.DiscardAll, null, ViewerClient.ShortTimeout, out _); public void Focus(PendingSnapshot snapshot) => - Send(ViewerVerb.Focus, snapshot.Key, out _); + Send(ViewerVerb.Focus, snapshot.Key, ViewerClient.ShortTimeout, out _); + /// + /// Hidden rather than quit, because this owner is holding the queue. Quit exits the process, + /// and the queue is in its memory, so one menu item meant "close the window" in the arrangement + /// where the tray owns the queue and "throw away every pending snapshot, without asking" in + /// this one - after which they were simply gone from the menu, a refused connection being + /// indistinguishable from nothing pending. + /// + /// Hiding leaves the process serving, which it has to be for the queue to survive at all, and + /// leaves the user where the other arrangement leaves them: no window, and everything still + /// pending. Focus brings it back. + /// + /// public void Close() => - Send(ViewerVerb.Quit, null, out _); + Send(ViewerVerb.Hide, null, ViewerClient.ShortTimeout, out _); - static bool Send(ViewerVerb verb, string? key, out string? message) + static bool Send(ViewerVerb verb, string? key, TimeSpan wait, out string? message) { message = null; - if (!Exchange(new(verb, key), out var response)) + if (!Exchange(new(verb, key), wait, out var response)) { message = "The snapshot viewer is not running."; return false; @@ -94,6 +144,6 @@ static bool Send(ViewerVerb verb, string? key, out string? message) return response.Ok; } - static bool Exchange(ViewerMessage message, [NotNullWhen(true)] out ViewerResponse? response) => - ViewerClient.TrySend(message, out response, wait: ViewerClient.ShortTimeout); + static bool Exchange(ViewerMessage message, TimeSpan wait, [NotNullWhen(true)] out ViewerResponse? response) => + ViewerClient.TrySend(message, out response, wait: wait); } diff --git a/src/DiffEngineTray/Tracker.cs b/src/DiffEngineTray/Tracker.cs index fe60f541..0aedf2e3 100644 --- a/src/DiffEngineTray/Tracker.cs +++ b/src/DiffEngineTray/Tracker.cs @@ -245,11 +245,18 @@ public Task Accept(PendingSnapshot snapshot) => // Gone, but not accepted. Told rather than logged, because the snapshot // vanishing from the menu otherwise reads as success. Log.Warning("Inline snapshot stale for `{Name}`: {Message}", snapshot.Name, message); - inlineFailed?.Invoke($"Could not accept the snapshot for '{snapshot.Name}'. {message}"); + inlineFailed?.Invoke(CouldNotAccept(snapshot.Name, message)); + break; + case AcceptOutcome.Unknown: + // No entry left to accept: it settled, or another surface got to it first. + // The menu is built from the last scan, so an item outliving its entry is + // ordinary rather than a failure, and a balloon here names a snapshot that + // is already in the source. The bulk path has always skipped these + Log.Information("Inline snapshot for `{Name}` was no longer pending.", snapshot.Name); break; default: Log.Warning("Inline snapshot accept failed for `{Name}`: {Message}", snapshot.Name, message); - inlineFailed?.Invoke($"Could not accept the snapshot for '{snapshot.Name}'. {message}"); + inlineFailed?.Invoke(CouldNotAccept(snapshot.Name, message)); break; } @@ -261,6 +268,13 @@ public Task Accept(PendingSnapshot snapshot) => } }); + // The owner does not always have something to add, and a balloon ending in a bare full stop + // and a space reads as a message that went missing + static string CouldNotAccept(string name, string? message) => + message is { Length: > 0 } + ? $"Could not accept the snapshot for '{name}'. {message}" + : $"Could not accept the snapshot for '{name}'."; + public void Discard(PendingSnapshot snapshot) { if (!inline.Discard(snapshot, out var message)) diff --git a/src/DiffEngineViewer.Tests/IpcTests.cs b/src/DiffEngineViewer.Tests/IpcTests.cs index efd2f1ce..12179326 100644 --- a/src/DiffEngineViewer.Tests/IpcTests.cs +++ b/src/DiffEngineViewer.Tests/IpcTests.cs @@ -138,12 +138,14 @@ public async Task AcceptTargetsTheKeyNotTheSelection() using var fixture = new ServerFixture(); fixture.Send(Inline(Fixtures.Patch())); fixture.Send(Inline(Fixtures.Patch("OtherTests.cs", 7, null, "new"))); - await Assert.That(fixture.Host.State.Selected).IsEqualTo(0); + // A patch that arrives is selected, so the newest is what is in view + await Assert.That(fixture.Host.State.Selected).IsEqualTo(1); - fixture.Send(new(ViewerVerb.Accept, QueueEntry.KeyForInline("OtherTests.cs", 7))); + // The one that is not + fixture.Send(new(ViewerVerb.Accept, QueueEntry.KeyForInline("SampleTests.cs", 42))); await Assert.That(fixture.Host.State.Queue.Count).IsEqualTo(1); - await Assert.That(fixture.Host.State.Queue[0].Name).IsEqualTo("SampleTests.cs:42"); + await Assert.That(fixture.Host.State.Queue[0].Name).IsEqualTo("OtherTests.cs:7"); } [Test] @@ -197,13 +199,15 @@ public async Task DiscardAll() public async Task FocusSelectsAndRaises() { using var fixture = new ServerFixture(); - fixture.Send(Inline(Fixtures.Patch())); fixture.Send(Inline(Fixtures.Patch("OtherTests.cs", 7, null, "new"))); + fixture.Send(Inline(Fixtures.Patch())); + // Each of those asked for the window as well, and this is about what the verb does + fixture.Windows.Clear(); var response = fixture.Send(new(ViewerVerb.Focus, QueueEntry.KeyForInline("OtherTests.cs", 7))); await Assert.That(response.Ok).IsTrue(); - await Assert.That(fixture.Host.State.Selected).IsEqualTo(1); + await Assert.That(fixture.Host.State.Selected).IsEqualTo(0); await Assert.That(fixture.Windows).IsEquivalentTo([WindowCommand.Focus]); } @@ -228,6 +232,8 @@ public async Task Quit() { using var fixture = new ServerFixture(); fixture.Send(Inline(Fixtures.Patch())); + // The patch asked for the window on its way in + fixture.Windows.Clear(); fixture.Send(new(ViewerVerb.Quit)); diff --git a/src/DiffEngineViewer/Ipc/MessageHandler.cs b/src/DiffEngineViewer/Ipc/MessageHandler.cs index f699702e..1ddc9f54 100644 --- a/src/DiffEngineViewer/Ipc/MessageHandler.cs +++ b/src/DiffEngineViewer/Ipc/MessageHandler.cs @@ -10,8 +10,16 @@ class MessageHandler(SessionHost host, ViewerActions actions, Action ViewerMessageHandler.Handle(this, message); - int IQueueOwner.Enqueue(InlinePatch patch) => - host.Mutate(_ => ViewerSession.EnqueueInline(_, patch)).Queue.Count; + int IQueueOwner.Enqueue(InlinePatch patch) + { + var count = host.Mutate(_ => ViewerSession.EnqueueInline(_, patch)).Queue.Count; + // Brought forward on the entry that arrived, which is what a tray owner does with one of + // these. Without it a patch landing in a window that is hidden - which this one is + // whenever a tray is running and the queue last emptied - showed up only as a tray icon + // on the next scan, and one landing in a window behind the editor showed up not at all + ((IQueueOwner) this).Window(WindowCommand.Focus, InlineKey.For(patch.SourceFile, patch.LineHint)); + return count; + } void IQueueOwner.Settle(string key, string? origin) => host.Mutate(_ => ViewerSession.Settle(_, key, origin)); diff --git a/src/DiffEngineViewer/Ipc/OwnerLink.cs b/src/DiffEngineViewer/Ipc/OwnerLink.cs index 02929d14..1d800e2c 100644 --- a/src/DiffEngineViewer/Ipc/OwnerLink.cs +++ b/src/DiffEngineViewer/Ipc/OwnerLink.cs @@ -87,6 +87,27 @@ public bool Pump(out bool sent) } public void Run(Cancel cancel) + { + try + { + Pump(cancel); + } + catch (Exception exception) + when (exception is not OperationCanceledException) + { + // This runs on a task nothing awaits until shutdown, so a throw here used to fault it + // unobserved and leave a live window showing a queue that had stopped being read - + // the worst of the available outcomes, since it looks exactly like a quiet queue. Said + // out loud instead, through the same channel an owner that went away uses + host.Mutate(_ => _ with + { + Message = $"The queue owner could not be read: {exception.Message}", + Exit = true + }); + } + } + + void Pump(Cancel cancel) { while (!cancel.IsCancellationRequested) {