Race condition in join lines?

[aochagavia]

If you execute join lines lots of times very fast, the resulting program is different than when you execute join lines the same amount of times with pauses in between.

Steps to reproduce:

1. Open ra_cli/src/main.rs
2. Place the cursor at the beginning of main
3. Press and hold Ctrl+Shift+J until the whole main function is folded in a single line

In the end you should probably see parse errors, because the program was transformed into something inconsistent.

If you press Ctrl+Shift+J manually in, say, intervals of 1 second, everything goes well.

[matklad]

I think the race condition is on the client side. Specifically, when we do this, we probably need to make sure, somehow, that the server is aware of all the changes that has been made on the client side, and that no new changes happen.

I am not sure what is the proper VS Code API to ensure correctness of edits, that is, to ensure that if an edit was generated at time t1, and is applied at time t2, no intervening changes happened.

I am slightly worried that the LSP protocol itself might not be correct with respect to ordering of events. The problem with the protocol is that it includes version numbers together with text document, but, for the edit to be valid, you need to know the global version number: if edit affects docs A and B, but the document C changes, the edit can still be invalid.

Compare this with the Dart protocol, which makes this guarantee:

There is no guarantee concerning the order in which responses will be returned, but there is a guarantee that the server will process requests in the order in which they are sent as long as the transport mechanism also makes this guarantee.

Dart protocol also includes file stamps, which are deprecated, presumably because they are not sufficient to maintain consistency.

So, TL;DR here is that we should study VS Code API to learn how to make concurrent edits correct.

[matklad]

Oh, it's even worse, LSP spec says

On the other hand, the server most likely should not reorder textDocument/definition and textDocument/rename requests, since the executing the latter may affect the result of the former.

That can't be right: it's the client that manages state of the world, the server simply can't be tasked with sending responses that are valid when the client receives them.

[matklad]

Filed microsoft/language-server-protocol#584 upstream.

[aochagavia]

Maybe we can use a workaround here? For instance, we could add a 200ms delay between actions or something like that. Though I hope there is a better solution...

[JohnHeitmann]

Confirmed this bug still exists. To repro I had to add a sleep on the server.

Here's what happens when the user requests joinLines twice faster than the server can respond once.

Client point of view:

-> joinLines1
-> joinLines2
<- joinLines1
<authoritative doc now updated>
-> TextDidChange1
<- joinLines2
<BUG! authoritative doc updated again, but edit refers to ranges in the old version, not current>
-> TextDidChange2

Server point of view:

-> joinLines1
<- joinLines1
-> joinLines2
<- joinLines2
-> TextDidChange1
-> TextDidChange2

You don't even need two joinLines requests. The bug should still exist if there's one joinLines outstanding, and the client workspace changes for any other reason that affects the region of the document being unwrapped.

At a minimum to avoid document-corrupting edits what I think should happen is that the client passes a doc version to the server, and the server passes back that same version. On response the client compares the current workspace version with the response's version, and discards it if there's a mismatch.

I prototyped that out, and half of it is super easy. The request types already support a VersionedTextDocumentIdentifier. It gets a little gross on the response. I'm guessing the protocol uses version for server task cancellation, not document consistency, since there are no version fields in the responses. I hacked up something at the top level of SourceChange to prototype the interactions, but if this is the direction to go I'll have to make some more extensive change to how SourceChange responses work. If you agree up to this point let me know and I'll get a pull request together.

Ok, that covers consistency, but leaves a bumpy user experience. If a user asks for 20 lines to be joined and only a fraction are, that can feel frustrating. As an enhancement, we could resend the joinLines request with the exact same params. This does have the danger that the old cursor position might now be invalid due to other workspace changes. So, if perfect correctness is the goal we shouldn't make this optimization. I'd lean towards not making this change just to keep complexity down.

Optimization: Server-side cancellation is an optimization opportunity. I don't think it's worth it. joinLines is cheap. Another optimization opportunity could be switching joinLines to be async on the server so it doesn't block other requests. If it's only a one liner with no other fallout that could be an easy tiny win. But, that's also probably not worth the effort of even benchmarking.

Other methods: Other methods might have the same problem. I'm not super familiar with the entire feature set of rust-analyzer. The server-side search and replace could have the same problem in theory. I don't think folks are banging on the search and replace key like they will with joinLines, so it might be harder to trigger and lower priority to fix. I'll play with it and see.

[matklad]

I can no longer reproduce this on master, and we've cleaned up our main loop a while ago