Semantic highlighting flickers

[matklad]

I've tried new semantic highlighting API in rust-analyzer (which was implemented by @kjeremy). I am super excited to see this feature finally in the LSP!

However, test drive shows this flickering:

I am not exactly sure, but I think the reason here is content modifies errors:

[Trace - 4:17:49 PM] Received response 'textDocument/semanticTokens - (437)' in 15ms. Request failed: content modified (-32801).

The editor asks for highlights, and then continues with sending modifications. Server than cancels the in-flight highlighting request. This causes the provider to return undefined, and that apparently clears the previous highlights. Because user continues typing, the highlighting is not re-applied until the user stops and the server is able to catch up.

It seems like the better behavior in this case is to just shift previous highlighting ranges, instead of clearing them. Not sure if this something to be fixed in vscode-languageserver-node, vscode itself, or the API for SemanticTokensProvider.

[rcjsuen]

It seems like the better behavior in this case is to just shift previous highlighting ranges, instead of clearing them. Not sure if this something to be fixed in vscode-languageserver-node, vscode itself, or the API for SemanticTokensProvider.

Monaco itself seems to be okay based on some random testing I just did in my editor so it may be something at the LSP level?

[matklad]

Found another related with content-modified cancellation: if highlighting is in progress and then some outside change occures (for example, file on disk changes), it gets canceled and is not retried. This is diffrenet from the case where the modification is triggered by the user, because in that case we retry and get the correct highlighting.

For rust-analyzer, the first case happens on startup: when we ingest project files from disk, we trigger internal modifications and cancel highlighting request. This specific case can and should be handled on our side by delaying requests until the whole project is loaded, but the general issue of missed highlighting due to unrelated changes still exists.

[dbaeumer]

The VS Code editor actually shifts the tokens automatically.

@alexdima what is the suggested way to inform the editor about a rejected sematnic token request. Should I simply rethrow or do you have a special error code that I can use to inform you to retrigger the request later (e.g. the busy case we discussed).

[alexdima]

the current implementation

Returning undefined is the same as returning an empty array and it will make the editor clear all semantic tokens, so that is what leads to the flickering. Currently, throwing an error with the message busy aka throw new Error('busy');/return Promise.resolve(new Error('busy')) will be swallowed by the editor (not logged to error telemetry) and the semantic tokens will not be dropped and a new request will be made. This is what our built-in TS semantic tokenization does when it cannot map back offsets to (ln;col) coordinates...

about the 'busy'

@dbaeumer I've talked with @jrieken and in the vscode API we don't have a clear way to specify this "busy" situation. You mentioned you also think LSP should have a "busy" concept, not sure what would be the way to express that in LSP? How should that be better expressed in the vscode API? I am not very happy with an error with a certain message...

[matklad]

Confirmed that throwing busy fixes the fickering: rust-lang/rust-analyzer#3339.

Also confirmed that it doesn't fix the second problem with the absence of retry behavior. Ie, the client sends one semanticTokens request, it fails with busy, and the file remains not highlighted until I type something, thus triggering the request again.

[alexdima]

@matklad You're correct. After an error, a new request is issued only if text changes occurred in the meantime -- https://github.com/microsoft/vscode/blob/8758dc9dddf95f358eb93c969353e5cf245ed1e4/src/vs/editor/common/services/modelServiceImpl.ts#L796-L810

But why do you throw busy if there are no text changes in the meantime? In other words, what would you expect to happen? Should the editor request the semantic tokens again even without more text changes? At what rate should that be? i.e. Should that be limited somehow? ... and why?

[matklad]

@alexdima the fundamental issue here is that highlighting of a file might be invalidated by changes to other files. For example, while the server computes the highlighting, the client might change git branches in the terminal (which modify some unrelated files), and the server gives up highlighting because it is now obsolete. Another hypothetical case is "I can't highlight this file right now, I am waiting for this library to be downloaded from the Internet".

In the specific case of rust-analyzer, this behavior is an artifact of an implementation. We have a single data structure, global state, which is protected by rw lock. Highlighting and other features work under r lock, any modification needs a w lock. When a new change arrives, we cancel all in-process r operations, which causes all pending requests to fail with "content-modified" error. One motivation for this is that, if the state has changed, the results of requests could be obsoleted anyway.

What is the correct retry behavior is an interesting question. The answer is clear in the push model: if the client registers an interest in a file, server responds with highlights as soon as quiescent state is reached (ie, there are no pending in-flight changes).

For pull model, I am not sure what's the right behavior. "retry indefinitely without delays", paradoxically, seems the theoretically right answer at least for rust-analyzer's architecture? Requests only get cancelled if modification arrives afterwards; if there are some pending modifications, and a new requests arrives, it's just scheduled after the modifications.

[jrieken]

For pull model, I am not sure what's the right behavior. "retry indefinitely without delays", paradoxically, seems the theoretically right answer at least for rust-analyzer's architecture?

For the pull model we usually solve this by allowing providers to have an event that signals a change. The CodeLensProvider is a sample for that

[Gama11]

That event still isn't available in the LSP though / requires nasty hacks. :( microsoft/language-server-protocol#192

[matklad]

Hit a very real case of stale highlighting right now:

[dbaeumer]

@jrieken we discussed that a while back ago and I somehow thought that having a busy indication to signal that a server is currently not able to answer the questions is a good idea. IMO this is even unrelated to having an event to invalidate persented data and ask the client to refetch the data.

@alexdima a model that would make sense to me is as follows:

• user changes text
• client fetches semantic tokens
• promise is resolved with busy. This indicates the data is not processed and the old semantic tokens are still valid
• client issues another request either after the user has typed or after a timeout
• if we want we can define that a server / extension can only report busy n times before it is ignored.

I would indicate Busy through a special Error class (comparable to FileSystemError).

I can of course simulated this with a change event. However this has the disadvantage that the exension has no knowledge about part visibility and therefore might trigger change events although no data is presented.

[jrieken]

@jrieken we discussed that a while back ago and I somehow thought that having a busy indication to signal that a server is currently not able to answer the questions is a good idea.

Sure, I am still in agreement with that but we decided to accept "any" error and not clear semantic highlights in its presence. However, I don't see how the error solves the case of two editors where typing in A causes changes in B - for that I can only image the event.

[alexdima]

With microsoft/vscode@c2a604a , I've added support for a change event from the provider.

[dbaeumer]

Regarding the busy indication: I discussed this with @alexdima again and noticed that naming it busy is pretty misleading. A better name would very likely be server side cancelation of an operation.

@jrieken: what is your take on this. However it still doesn't solve how we would communicate this (e.g. using an exception or a special return object).

[matklad]

One possible way out of this design question is to hide the retry loop inside the vscode-langaugeserver-node. Ie, the VS Code provider in this repository can retry the LSP query itself instead of propagating busy outwards. if VS Code itself correctly cancels uncompleted provider calls after file modifications, this should probably work out.

[dbaeumer]

I still think that this might not work since even the LSP library needs to give up after some tries and the problem remains what to return then. If the provider has registered an edit provider I think it is easy since we could return an empty edit. If it didn't then all that remains is throwing an exception. So I would still like to agree on what to do if a NON edit request times out. One possible idea would be to allow data in SemanticTokens to be undefined. If we would then allow to signal a change event on a specific file I think I could implement that savely in the lib. @alexdima what is your take on this?

[alexdima]

I don't think vscode-languageserver-node can run a retry loop.

The VS Code request clearly expects the reply for a certain project state, the project state at which the request was made.

The whole point of the server canceling is that the server believes it makes no sense to answer the request at that certain state and the client should recreate a request at a new project state. This is nothing vscode-languageserver-node can do.

For example, for semantic tokens, when a request is made, text buffer edits are collected, such that when the response comes back, the response is adjusted against the user edits that occurred in the meantime. When a request is canceled (completed via a busy error), the collected text buffer edits are dropped and a new request is made, which is expected to return results at that new state.

[dbaeumer]

@alexdima makes perfect sense. So we don't to agree on how to cancel. Should that be an exception or data in SemanticTokens to be undefined. I would actually prefer an exception.

[alexdima]

@jrieken @dbaeumer Here is an idea. Currently, when a request is canceled by the client, we kind of expect and ignore (in error telemetry, etc.) any error with the message Canceled.

So what if we just extend this and allow that a Canceled error could be thrown even when the cancellation token is not triggered by the client... Then a server side cancelation could be detected by checking if the error is Canceled and if the cancelation token is not set.

[jrieken]

we kind of expect and ignore (in error telemetry, etc.) any error with the message Canceled.

Well, only kind of. The canceled-error is usually generated by a utility that we own and generally all provider triggered errors are ignored/dropped.

Anyways, it doesn't invalidate your idea of supporting "cancellation from both ends", e.g today only client cancels but with that the server can also cancel. But it does require us to spec this kind of error.

[dbaeumer]

@jrieken @alexdima since we want to finialize the API on this in March I would like to find a solution for this.

IMO the cleanest way would be to have a CancelError like we have FileSystemError. This is IMO better than using a special message

[alexdima]

Tracking in microsoft/vscode#93686 ... We will bolt it on after March since technically it isn't a breaking change, we will for now support errors with the message 'busy' and then we will introduce a new error type and then finally, in 6 months or so remove support for 'busy' messages... Not the best outcome, but there was not enough time to get agreement on an error name, etc. much less make it finalized.

[dbaeumer]

@alexdima thanks.

[kjeremy]

@matklad do we still need the workaround:

async provideDocumentSemanticTokens(document: vscode.TextDocument, token: vscode.CancellationToken, next: DocumentSemanticsTokensSignature) {
                const res = await next(document, token);
                if (res === undefined) throw new Error('busy');
                return res;
            }

Now that microsoft/vscode@c2a604a is in?

[matklad]

I din‘t think so

…

On Sunday, 9 August 2020, Jeremy Kolb ***@***.***> wrote: @matklad <https://github.com/matklad> do we still need the workaround: async provideDocumentSemanticTokens(document: vscode.TextDocument, token: vscode.CancellationToken, next: DocumentSemanticsTokensSignature) { const res = await next(document, token); if (res === undefined) throw new Error('busy'); return res; } Now that ***@***.*** <microsoft/vscode@c2a604a> is in? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#576 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M24D6UFVH7EUFL5WPDR74HETANCNFSM4K4IJASQ> .

[dbaeumer]

@kjeremy but the event is not yet in the protocol.

[dbaeumer]

@alexdima do we have a final story for the busy (e.g. a special Exception)

[alexdima]

Not fully finished -- microsoft/vscode#93686

[dbaeumer]

@alexdima can we make progress on this in September so that I can do that in LSP.

[alexdima]

@jrieken To be honest, this is more on your plate, since such an error would be relevant to any of the other providers. Would you be willing to drive this since it has so much more impact than the semantic tokens provider?

[dbaeumer]

@jrieken any additional insights here?

[jrieken]

No update, work will happen here: microsoft/vscode#93686

[dbaeumer]

Moving to 3.17 since no progress in VS Code so far.

[kjeremy]

@dbaeumer does this fix the issue? dae62de

[dbaeumer]

@kjeremy yes it should have improved it. I still need to convert the code to the new CancelError from VS Code API.

[dbaeumer]

Throwing new CancellationError() now.