Fix provisional completion corrupting other requests #11665
Conversation
…g" for the right version
…er requests wait for us to revert
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/GeneratedDocumentPublisher.cs
Show resolved
Hide resolved
| commitChar: '\t'); | ||
| } | ||
|
|
||
| private async Task VerifyTypeAndCommitCompletionAsync(string input, string output, string search, string[] stringsToType, char? commitChar = null, string? expectedSelectedItemLabel = null) |
There was a problem hiding this comment.
This method just moved down to below all of the actual tests.
| TestServices.Input.Send("."); | ||
| TestServices.Input.Send("n"); |
There was a problem hiding this comment.
Full disclosure: This test on my machine doesn't type fast enough to repro the issue reliably, but it did do it sometimes.
There was a problem hiding this comment.
Probably worth adding a comment on the test that if it's flaky the cause might be a regression in what you fixed. That way future Dave™️ remembers
There was a problem hiding this comment.
Not sure I follow. The disco colors detection is now in every test essentially, so this test really just tests provisional completion. If any test causes disco colors, that test will fail so the scenario that caused it will be in the name of the test.
There was a problem hiding this comment.
I'm re-reading my comment, and I think I should expand: This test had flakiness in terms of false-positives (ie, it would sometimes pass) before my code change. After my code change I don't expect it to fail or be flaky.
| UpdateVirtualDocument(provisionalChange, request.ProjectedKind, request.Identifier.Version, hostDocumentUri, virtualDocumentSnapshot.Uri); | ||
| // We update to a negative version number so that if a request comes in for v6, it won't see our modified document. We revert the version back | ||
| // later, don't worry. | ||
| UpdateVirtualDocument(provisionalChange, request.ProjectedKind, -1 * request.Identifier.Version, hostDocumentUri, virtualDocumentSnapshot.Uri); |
There was a problem hiding this comment.
maybe a pathological case, but for small versions is there any concern? E.g we're setting this to -1 for v1. Could updates cause some sort of collision there? Should this just be int.MinValue + request.Identifier.Version?
There was a problem hiding this comment.
So it needs to be less than any other version, so that when something comes in it doesn't think "oh, there is a newer version, I'll cancel." It could be MinValue, but I thought it actually looks nice in the logs that you can see which version caused the provisional update.
The only possible conflict would be two provisional completions at the same time but were protected by the UI thread from that.
Oh and not to toot my own horn but this situation is impossible to hit in cohosting 😝
| TestServices.Input.Send("."); | ||
| TestServices.Input.Send("n"); |
There was a problem hiding this comment.
Probably worth adding a comment on the test that if it's flaky the cause might be a regression in what you fixed. That way future Dave™️ remembers
When we wait for completion in UI tests, we're causing a lot more UI thread contention that in the real world, and it makes the test very flaky
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/GeneratedDocumentPublisher.cs
Show resolved
Hide resolved
...nguageServices.Razor/LanguageClient/Endpoints/RazorCustomMessageTarget_UpdateCSharpBuffer.cs
Outdated
Show resolved
Hide resolved
...nguageServices.Razor/LanguageClient/Endpoints/RazorCustomMessageTarget_UpdateCSharpBuffer.cs
Outdated
Show resolved
Hide resolved
...nguageServices.Razor/LanguageClient/Endpoints/RazorCustomMessageTarget_UpdateCSharpBuffer.cs
Outdated
Show resolved
Hide resolved
...nguageServices.Razor/LanguageClient/Endpoints/RazorCustomMessageTarget_UpdateCSharpBuffer.cs
Outdated
Show resolved
Hide resolved
|
I ended up taking out the disco color detection in every test. Will merge this to get the fix in for P3 though, as at least the provisional completion won't cause it any more. |
Fixes #11385
Provisional completion was causing other requests to see corrupted documents because a) it re-used a version number that was already used for the original document and b) when updating the C# buffer we didn't check to ensure we were on the right version to begin with. Now we update the version to a negative number for provisional completion (eg, v -5) and then when we try to update the buffer from 5 to 6, instead of applying an edit to a document that we think doesn't have a
., but does because we added it, we instead wait for the version to be reset back to 5, then update to 6.Additionally, disco colors was happening because in there somewhere we got a semantic tokens request. This could have either happened for v5, but it could have been a v5 that had the extra
., or it could have been for v6, which could have been a corrupted document because the changes were applied before the provisional update was reverted.Honestly surprised this ever worked. The only explanation I can find is that we used to have such worse perf that everything just overlapped less :)