BUGFIX: Fix semantic highlight scheduling

[qchateau]

If provider.provideDocumentSemanticTokens returns
null (which is the case when the server cancels the
request with the error code ContentModified) then
the semantic token call would not be rescheduled
even if there were pending changes.

This commit makes sure the call is always rescheduled
if there are pending changes.

[ghost]

All CLA requirements met.

[alexdima]

@qchateau There is some misunderstanding here.

If a provider is busy, then it should not resolve the resulting promise with null.

It should reject the resulting promise with an error. As of right now the error name needs to be busy, but with #93686 we are looking that a special error type can be thrown by the provider.

An example can be seen in the TS semantic tokens provider -- https://github.com/qchateau/vscode/blob/76c22d48c823ba0b78405968a05a4493e08fa7b6/extensions/typescript-language-features/src/languageFeatures/semanticTokens.ts#L98

[qchateau]

I'm not sure how you interact with the guys of vscode-languageserver-node but the issue is that your codes don't match.

See this: https://github.com/microsoft/vscode-languageserver-node/blob/960c801b4fb45acf6bf47f919a4573e902a7eb84/client/src/common/client.ts#L3664

On error, if the request is cancelled or if the error code is "ContentModified", then a defaultValue is returned. In the case of semanticHighlighting, it's null. Leading the the problem I described (tldr: the request is not retried)

@dbaeumer

[dbaeumer]

@qchateau the token in the code you are pointed to is the client cancel token. That means the client has canceled the request (for example since the file got closed) and hence the library returns a default value (the client will ignore the value anyways). If the response is canceled from the server (e.g. error.code === LSPErrorCodes.RequestCancelled) and the client token is not canceled then it throws a special cancel error that VS Code should treat accordingly. So if you as a server implementer want to cancel a semantic token request from the server simple return an error response with the error.code set to LSPErrorCodes.RequestCancelled.

[qchateau]

I'm talking specifically about the case where the error code is LSPErrorCodes.ContentModified.
As far as I understand, LSP servers are allowed to return that error, expecting a retry from the client.

In clangd, we use this code if the client sends a didUpdate after another request, that we then want to cancel because the content is outdated. I think this is the expected behavior on the server side.

The vscode guys do retry if they get an error OR a non-null response...which makes this awkward...

[dbaeumer]

@qchateau The spec says that it is up to the client to decide to resend the request. I think it would make sense to resend it for semantic tokens since there we can trigger the request through VS Code.

Other requests (mostly the once that are position based) can not easily be resend since there is not way to decide which position to use nor do you know if the client might take the result and adopt it to typings that happened. For example VS Code adopts a completion result to typings in the editor. So I would in this case strongly argue to not cancel the request.

[dbaeumer]

Actually I take back what I wrote above since the LSP strategy is different around content modified:

it is up to the client to cancel a request if a result is useless for the client after the content of the document (or resource the request refers to) has been modified. It is not the servers responsibility to decide this. As said above a client might still take the result and modify it to make it work with the current resource state.

[qchateau]

From the LSP spec:

if a server detects a state change that invalidates the result of a request in execution the server can error these requests with ContentModified. If clients receive a ContentModified error, it generally should not show it in the UI for the end-user. Clients can resend the request if appropriate.

From what I understand, the server is allowed to cancel on its own, and that is the surprise of this error code

[dbaeumer]

And from the spec a bullet point above :-)

if a client sends a request to the server and the client state changes in a way that the result will be invalid it should cancel the server request and ignore the result. If necessary it can resend the request to receive an up to date result.

Yes, the server is allowed to cancel on its own but it is not the recommended way. The recommended way is that invalidating a result is up to the client. I will clarify that in the spec.

[dbaeumer]

One additional thing: VS Code will make use of the semantic token result even if the state of the document changed. It will apply the state change to the result and will send a new request. If a server would cancel the request the client might for a long time not update the semantic tokens if the user types with a decent speed.

[qchateau]

Are you saying we should ignore the spec ?

[dbaeumer]

No, but the spec recommends to wait for a client to cancel since a server can not decide whether the result is useful for the client or not. I will make that more clear since I have to agree that was not clearly articulated.

[dbaeumer]

I clarified the spec. An additional note: if a server detects that some internal state changed and it needs to re-compute all semantic tokens then a sever can send a request from the server to the client to force such a calculation.

[qchateau]

I still think my PR is relevant as vscode should not treat !token as a special case and should reschedule if needed anyway.

I'll try to push an update to clangd to return an outdated result instead of aborting with ContentModified so the problem won't happen anymore.

[dbaeumer]

If this should be treated special then it has to happen in the LSP libraries anyways. The VS Code API is clear about this and it doesn't have ContentModified. This is an LSP and not a VS Code concept. The VS Code API requires that a special exception is thrown to retrigger a request.

[qchateau]

VSCode will retrigger the request on success as well if needed ... except if the result is null
Which is what this PR is about.

If I understand your latest comment correctly, you also suggest that vscode-languageserver-node you throw on ContentModified ?

[alexdima]

I still think my PR is relevant as vscode should not treat !token as a special case and should reschedule if needed anyway.

I agree. I have pushed a test to capture the case.

[alexdima]

Thank you!

[dbaeumer]

If I understand your latest comment correctly, you also suggest that vscode-languageserver-node you throw on ContentModified ?

Yes, since content modified is not known to VS Code API.

[sam-mccall]

@dbaeumer

No, but the spec recommends to wait for a client to cancel since a server can not decide whether the result is useful for the client or not.

Just as it is important for clients to be robust to naive servers (and LSP accommodates this), servers need to deal with imperfect clients.

The ability to reject requests with ContentModified is important to prevent an ever-growing backlog of file versions that need to be parsed. In particular we have seen clients in the wild that can send implicit requests after each keystroke and never cancel them. This causes a backlog when reparses are slower than the edit frequency.

Of course it's sufficient for either the client or the server to be smart, but absent some capability advertised by the client, servers must assume clients are dumb (and vice versa).

[dbaeumer]

@sam-mccall I only partly agree since a server reacting like this will very likely produce missing results for the user since if a client doesn't cancel it will very likely not resend the request either.

And instead of adding this logic to every server we would be better off fixing the clients. I can understand that you want to protect your server against this but my recommendation is to throttle and not to drop.

I will add a capability to the client that signals proper cancel behavior.

[sam-mccall]

if a client doesn't cancel it will very likely not resend the request either

Yeah :-( The motivating case was codeAction, where dropping the request entirely seemed OK.

my recommendation is to throttle and not to drop.

We have to reject/drop requests in some scenarios to avoid infinite backlog. But instead of doing this when the queue says [read,edit], maybe it's enough for us to do it when the queue says [read,edit,read] and the reads are similar (e.g. two semanticTokens requests). Then we'd never cancel if the client is smart enough to leave only one request outstanding.

I will add a capability to the client that signals proper cancel behavior.

Thanks!

instead of adding this logic to every server we would be better off fixing the clients

I daresay if I dealt mostly with one client which talked to N servers, rather than the reverse, I'd say the same :-)

[dbaeumer]

We have to reject/drop requests in some scenarios to avoid infinite backlog. But instead of doing this when the queue says [read,edit], maybe it's enough for us to do it when the queue says [read,edit,read] and the reads are similar (e.g. two semanticTokens requests). Then we'd never cancel if the client is smart enough to leave only one request outstanding.

I think that this might be problematic as well, especially if the first result might clear some UI and the client has no support to distinguish between empty result and no valid result. You might be better off returning the last result.

instead of adding this logic to every server we would be better off fixing the clients

I daresay if I dealt mostly with one client which talked to N servers, rather than the reverse, I'd say the same :-)

It was meant as a general statement and I think a client capability will help since it motivates clients to think about this.