Rust analyzer options don't apply in VS Code until restart or settings refresh

[nokola]

VS Code stable 1.44.0 + rust-analyzer: nightly (080c983) 2020-04-08

Steps to reproduce

1. Open VS code with some Rust project
2. In settings.json add:

    "editor.tokenColorCustomizationsExperimental": {
        "*.mutable": {
            "fontStyle": "underline"
        }
    },
    "rust-analyzer.updates.channel": "nightly"

3. Install whatever is required from rust analyzer. Wait for inlay hints to show up.
   They show up OK:
   

4. Open Settings, add the below and save:

"rust-analyzer.inlayHints.parameterHints": false

Observed:

parameter inlay hints stay incorrectly

Expected:

parameter inlay hints disappear

Notes

1. Update some unrelated settings e.g. change the underline fontStyle to 'bold' and save settings, inlay hints will pick up.
   

2. Very confusing issue especially if settings updated through UI checkboxes (Ctrl+,)

VS Code:
Version: 1.44.0 (user setup)
Commit: 2aae1f26c72891c399f860409176fe435a154b13
Date: 2020-04-07T23:31:18.860Z
Electron: 7.1.11
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Windows_NT x64 10.0.18363

[Veetaha]

The problem is that the server updates its config with some latency.
When the config change event is triggered in our extension code and we refresh the inlay hints the server may not have updated its config at this point in time...
The smallest and dirties workaround would be to add sleep(someHeuristicNumberOfMilliseconds) before refreshing the hints.
The proper solution would be to wait for the response from the server on the config update request and only then propagate the config change event throughout or frontend extension.

I'll go for the proper solution, bear with me.

[Veetaha]

Well, this is not that easy as I've expected.

The problem is with the config update protocol.
The client only kindly sends a notification to the server about the config update. If I am not mistaken, by protocol the server is not required (or even must not) return a response for the notification and even if it could, this wouldn't help.

The server queries the config in a separate request right after receiving the notification according to our implementation. Only after that, we can consider the server to have the up-to-date config.

The pull model doesn't allow fixing this sync problem. We cannot wait for the notification response from the server and we can not wait until the server receives the configuration response.

I think we are left only with the workaround of adding a sleep() or adding a custom notification from the server that it has updated the configuration...

[nokola]

Interesting, thanks for writing the description of the issue. I'm not sure I understand what's going on.
This is what I expect would happen:

1. Human opens VS code settings, current config state is Config1
2. Human checks a box, which now changes config state to Config2 in Client
3. Client sends notification to Server
4. Server reads Config2 and updates

What I observe is that in step 4 above, server reads Config1 instead of Config2.
Few questions:

1. Can we use the above expected algorithm to get things working? I don't understand why the server needs to send back a notification about the config update.
2. Do you have a pointer in the source code where the config update in client and notification to server happens? Curious to see how it's implemented.

Thanks!

[nokola]

Just writing for my own clarification:
Oh, I see from your PR this is likely what's happening:

1. Human opens VS code settings, current config state is Config1
2. Human checks a box, which now changes config state to Config2 in Client
3. Client sends notification to Server
4. Client asks Server for updated hints <--- bug - race condition!
5. Server reads Config2 and updates

We need a way to ensure step 4. doesn't race with 5. which is what the Sleep(10) you added would do in some cases. 4. should execute after 5.

[Veetaha]

@nokola sorry for spitting the technical details, these are for the fellow contributors actually. I've already created a workaround PR #3948

Since you are curious, the problem is with the pull-based configuration update model that language server protocol introduced in microsoft/language-server-protocol#299

So the current workflow is:

1. User updated the settings.json
2. Vscode extension handles the config change event and sends a notification to the language server without supplying the current config object: https://github.com/rust-analyzer/rust-analyzer/blob/997c959d4f373df0cbba09609c5e4d8215d3d0c7/editors/code/src/main.ts#L101-L105
3. Language server handles the notification and issues a separate request for the new config object: https://github.com/rust-analyzer/rust-analyzer/blob/997c959d4f373df0cbba09609c5e4d8215d3d0c7/crates/rust-analyzer/src/main_loop.rs#L604-L618
4. VScode extension handles the request via the TypeScript library vscode-languageclient and returns the current configuration obejct.
5. The language server receives the response and updates its own configuration https://github.com/rust-analyzer/rust-analyzer/blob/997c959d4f373df0cbba09609c5e4d8215d3d0c7/crates/rust-analyzer/src/main_loop.rs#L357-L377

With this workflow, the vscode extension doesn't know the exact time when the step 5 happened so that it can refresh the inlay hints.

So I've just added a heuristic 10 ms latency before updating the inlay hints, so that we give the language server the time to update its config.
Another (maybe a bit more confident) workaround (but this involves more code) would be to add step 6 where the language server responds with the notification that it has updated its config.

[nokola]

Thanks for the detailed explanation! I just downloaded rust-analyzer's code yesterday and still learning about it.
I'm wondering if this could be a better fix:

1. Human opens VS code settings, current config state is Config1
2. Human checks a box, which now changes config state to Config2 in Client
3. Client sends notification to Server
4. Client asks Server for updated hints <--- bug - race condition! (to be fixed by 4.1_NEW below)
   4.1_NEW. Server checks if it's in the middle of reading configuration. It is since it received notification in step 3.
   4.2_NEW. Server finishes reading Config2
   4_3_NEW. Server responds with inlay hints for Config2, as expected

In the above flow, we keep the pull model, it's just that the server ensures it always has the latest config when it starts handling a request.

Another possible fix: treat the configuration settings as "world stopping for a short time" in the server:

1. Human opens VS code settings, current config state is Config1
2. Human checks a box, which now changes config state to Config2 in Client
3. Client sends notification to Server
   3.1_NEW. Server blocks processing of all further notifications until it reads Config2
4. Client asks Server for updated hints no race condition anymore
5. Server responds with inlay hints based on Config2

What is your opinion? (also if you have no time to answer a noob question, np! I can look at the source!)

Edit: train of thought: it looks like the server executes requests out of order, when in the case of config update, all requests sent after the config update should execute after the config update in the server. Interesting, this is similar to a memory fence in multithreading.