Inline bugs - #764
Merged
Merged
Conversation
U+0085, U+2028 and U+2029 are line terminators to the C# lexer, and RenderRegular passed all three through as themselves: its escape branch covers the C0 range and DEL, none of which they are. Snapshot content holding one rendered to source that does not compile - CS1010 in a regular literal, and CS8999 in a raw one, where the line the terminator starts no longer carries the closing delimiter's indentation. Both confirmed against the compiler. No delimiter width helps with the raw form, so content holding one falls back to a regular literal, which is the shape F# already falls back to for a quote run it cannot widen past. The fallback sits in RenderRaw so a caller asking for the raw form directly gets it too. F# is held to the same rule. Nothing says its lexer reads these as line breaks, but the two languages writing the one shape is what RenderMultiLineMatchesCSharp asserts and what the readers are written against. The fsi round trip covers the escaped form, so the belief that F# reads \uXXXX back as the one character is checked rather than assumed.
The \U escape checked only the top of the Unicode range, so a lone surrogate reached char.ConvertFromUtf32 and threw. Nothing between there and the process applying the patch catches, and a TryParse that throws is not one that failed: the callers handle false. Both languages had it, and both now ask IsScalarValue. Also read @"""x""" as the verbatim literal it is. The quote run was measured before the @ was considered, so a verbatim literal opening on an escaped quote looked like a raw string carrying an @ and was rejected. F# already asked in that order, with the reasoning written out; C# now matches it, which is why the F# tests already covered the case. Verified against the compiler: @"""x""" is valid C# worth "x".
Writing in place truncates the file and fills it back in, so a process killed partway through, or a disk that fills up, cost the caller the rest of their source file. The mutex keeps two appliers apart but says nothing about one that stops halfway. Everything else here is careful because the whole file is rewritten rather than the patched span; the write was the step that could still lose it. The new content goes to a sibling temporary and File.Replace renames it over the original, which keeps the destination attributes and is on every framework this targets, unlike the overwriting File.Move. That moved the File.Exists precondition. Replace takes the path away for the instant it renames over it, and the check sat outside the lock, so an applier waiting its turn on a file another was finishing with reported it missing - which ParallelAppliesToSameFile went red on, intermittently, once the rest of the class was loading the machine. It is now asked inside the lock, with every other operation on the file. ContentIsNeverObservedHalfWritten reads alongside an apply and fails only if a length that belongs to neither whole file is seen; it goes red on the old write. The contention test pins the contract rather than the window, which is too narrow to reproduce on demand.
Every verb the remote host sends used the short wait, which is sized for the scan timer and the menu. An accept is not: the owner applies through InlineApplier, which waits up to ten seconds on its cross process mutex, and an owning viewer does that inside its session. Anything slower than half a second was reported as "The snapshot viewer is not running." while the owner was in the middle of writing the source file - and the snapshot then left the menu a scan later, contradicting the balloon. Accept and AcceptAll now use the same fifteen seconds InlineQueueClient and OwnerLink already use, and for the reason both of them state. Safe because accepts run on a worker, which is what Tracker.Accept exists to arrange; the listing and menu verbs keep the short wait. Both tests drive a real socket with an applier that sleeps past the old wait, and both go red on it.
The tray menu is built from the last scan, so an item outlives its entry whenever the test re-ran and passed or another surface accepted it first. Clicking one of those returns Unknown, which fell through to the failure branch and raised "Could not accept the snapshot for X. " - naming a snapshot that is already in the source, and with a null message leaving the sentence hanging. Unknown is now its own case and says nothing, which is what accepting a group has always done with it. The failure text is built by a helper so an owner with nothing to add does not produce the trailing full stop and space either.
Both accept clients decide what became of an accept by asking, in a second round trip, whether the entry survived. A listing that could not be made returned no items, and no items was read as "not pending", so an owner that took the accept and then went away - or answered with an error - came back as Accepted. A surface told that stops offering a snapshot nothing ever confirmed landed. TryList and TryListKeys now report false for an error response as well as for an absent owner: an empty item list on an error is not a statement about what is pending. StillPending answers three ways instead of two, and the case it gained maps to Unknown, which is the outcome that claims nothing. RemoteInlineHost does the same, and keeps flattening it to nothing pending for List, which is what the menu wants. The residual is the other direction and is inherent to inferring from a listing: a re-run that re-enqueues the same key between the accept and the listing still reads as Failed. Carrying the apply status on the wire would remove that and both round trips, but that is a protocol change rather than a fix.
Reuse was a scan of every already resolved directory for one the file sat under, taking the longest. Nothing looked between the file and that directory, so a file inside a nested solution was handed the outer one: the outer directory encloses it too, and only the order patches happened to arrive in decided which was cached first. The entry then showed under the wrong solution in the tray and the viewer. The cache is now consulted level by level on the way up, keyed by directory rather than by file, so the nearest solution is always the one that answers and a known directory still stops the walk without touching the disk.
Applying happens outside the host's lock and can wait ten seconds on the cross process mutex, and Accept throws the completion away if the entry changed while that was going on. But every fold rebuilt the entry, so a still failing test re-sending the byte identical patch inside that window counted as a change: the patch reached the file and the entry stayed pending with nothing reported, until something else settled it. A fold that adds nothing now hands the same variants back, so the accept recognises the entry it started on. The status of the last attempt is still dropped, which is what rebuilding did and what a re-run should do.
AReplacedEntrySurvivesTheAcceptItInterrupted re-queued the call site with the content it already had, so it was asserting the guard against a re-run that had not in fact replaced anything - which is the case the previous commit deliberately changed. It now re-queues different content, which is what its own summary describes, and the identical arrival gets its own test beside it. Amends the previous commit rather than reporting green over it: that commit left this red.
Two ways an owning viewer behaved unlike a tray owner, both of them the window. A patch arriving asked a tray owner for the window - "with no window open starts one; with one, this is the focus" - and asked an owning viewer for nothing. That window is hidden whenever a tray is running and the queue last emptied, so a newly failing test landed somewhere nobody could see and surfaced only as a tray icon on the next scan. "Close snapshot viewer" sent Quit, which exits the process. For a tray owned queue that is a display closing. For an owning viewer the queue is in that process's memory, so one menu item quietly threw away every pending snapshot - and they were then simply missing from the menu, a refused connection listing the same as nothing pending. It asks to hide instead, which leaves the process serving and the user where the other arrangement leaves them. IInlineHost.Close already said the queue was unaffected either way; now it is.
Three of them encoded the old behaviour incidentally rather than on purpose: they queued patches as setup and then asserted over a window command list that queueing no longer leaves empty, or over a selection that queueing no longer leaves at zero. FocusSelectsAndRaises and Quit clear the window list after their setup, so each still asserts exactly what its verb raised. AcceptTargetsTheKey- NotTheSelection now accepts the entry that is not selected - with the newest arrival selected, accepting it was no longer the case the test exists for. Amends the previous commit, which left these red.
Two ways the line oriented format could be taken at its word when it should not have been. sourceFile and framework ride the payload as themselves rather than base64, so a line break in either ends its line and everything after it is read as more of the payload: the six fixed lines shift, or one of the tolerantly read trailing fields is forged. Both are public settable properties, and a path may hold a line break off Windows. Build refuses rather than emitting one, since no shape of this format carries it - base64 there would be a version bump and a reader that predates it. mode went through Enum.TryParse, which takes a number as readily as a name, so "mode: 7" arrived as an InlinePatchMode that is none of them and then fell through every mode check in the patcher to behave as a Set. IsDefined settles it; the numbers that do name a mode still read.
Two paths differing just in case are one file on Windows and two on Linux, and the key folded unconditionally: on Linux the second patch took over the first's entry, and settling either settled both. Folded on Windows and macOS, left alone elsewhere. Every process addressing a queue is on the one machine, so they agree about which applies. KeyFormat pinned the folded form with mixed case inputs, which would now be wrong on the Linux leg of CI rather than right everywhere. Its cases are lower case now, so it pins the separator and the line, and the folding has a test per platform beside it.
ProcessViewerLauncher overwrote its Process on every relaunch without disposing the one before it, over a tray lifetime measured in weeks. Program disposed the server task with a using. Task.Dispose throws for a task that has not completed, so anything thrown between starting it and the await at the end of the method came out as an InvalidOperationException about the wrong thing. A task needs no disposal; cancelling ends it. OwnerLink.Run had no catch, and it runs on a task nothing awaits until shutdown. A throw faulted it unobserved and left a live window showing a queue that had stopped being read - which looks exactly like a quiet queue, so it is the worst of the outcomes available. It now reports through the same channel an owner that went away uses. No test: the known throw sources inside the pump are already defended, and there is no seam to inject one through that would not be testing the seam.
Connections were accepted and read one at a time, and the read ran until the client closed its stream. A test process that connected and stopped partway through writing therefore held the accept loop for as long as it stayed that way, and every move and delete from every other process on the machine went nowhere. Nothing timed it out. Each connection is now handled on its own task, which is how ViewerServer has always taken its connections, and the read has ten seconds to finish. Concurrency is safe here: the callbacks land in the tracker's concurrent collections, which the viewer port already writes to off this thread. Nothing awaits the per-connection task, so it reports what it catches rather than throwing into nowhere.
The raw string scan looked for a closing run of quotes and treated holes as content, so a literal inside one ended the outer string at its delimiter. Where the runs happen to pair up evenly the rest re-pairs and lands back on its feet, which is why this took a four-quoted literal holding a run of three to show: the last run then opens a string that runs to the end of the file, and the Snapshot call after it is inside that string as far as every search is concerned. Holes are now skipped whole, through the TrySkipHole the regular interpolated branch already uses - it lexes nested literals, char literals and comments properly. A run of fewer braces than dollars is still content, which is how a raw interpolated string carries a literal brace. F# is left alone deliberately. Its triple-quoted delimiter is exactly three and cannot hold a run of three, so its runs always pair and the mis-pairing this fixes has no way to start.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.