Reload part of the server configuration without restarts #3666

SomeoneToIgnore · 2020-03-20T22:41:56Z

Partially addresses #2857

Reloads all server configuration that's not related to VFS without restarts.

The VFS-related parameters are not considered, since VFS is planned to be rewritten/replaced in the future and I have a suspicion that with the current code, swapping the VFS and the file watchers on the fly will cause big troubles.

I have to store and process the config request id separately, since the workspace/configuration response returns any[] (https://microsoft.github.io/language-server-protocol/specifications/specification-current/#workspace_configuration), if there's a better way to handle those responses, let me know.

crates/rust-analyzer/src/world.rs

crates/rust-analyzer/src/main_loop.rs

Veetaha

LGTM, though the protocol for config reloading is somewhat imperfect...

crates/rust-analyzer/src/main_loop.rs

editors/code/src/client.ts

editors/code/src/inlay_hints.ts

editors/code/src/main.ts

crates/rust-analyzer/src/world.rs

SomeoneToIgnore · 2020-03-23T23:21:05Z

I would really appreciate if somebody can review and merge this before the #3696 lands :)

Maybe @matklad ?

Veetaha · 2020-03-23T23:28:32Z

Ah, sorry, there wil be a conflict ;)
[UPD] Hmm, I think there won't be one since we modified different lines of code.

crates/rust-analyzer/src/main_loop.rs

edwin0cheng · 2020-03-24T14:50:49Z

I just tested, at least "rust-analyzer.highlightingOn" do not works. (No reload notification or do not change any highlight)

matklad · 2020-03-24T14:54:27Z

Highlighting may be broken for some other reason. I think we should just delete old, non semantic-tokens based code already.

…

On Tue, 24 Mar 2020 at 15:51, Edwin Cheng ***@***.***> wrote: I just tested, at least "rust-analyzer.highlightingOn" do not works. (No reload notification or do not change any highlight) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3666 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M2T3TQ4COZEGKUA46DRJDCFRANCNFSM4LQV3ECA> .

edwin0cheng · 2020-03-24T15:03:39Z

@matklad Agreed.

OTHO , And I also tested:

inlay hints and parameter hints that both works correctly.
rust-analyzer.cargo-watch.enable works, but if I disable it during cargo checking, the progress bar displayed in VSCode does not clear.

crates/rust-analyzer/src/main_loop.rs

SomeoneToIgnore · 2020-03-24T20:22:01Z

Thanks a lot for testing, nice catches.

I've moved the whole highlighting section under the reload check, so the prompt now appears for all of them.
If I got that right, with semantic tokens feature on, we highlight the documents unconditionally, so that setting is part of the old highlighting mechanism.
I wonder, should we provide something similar to the new one too?

As for the check interruption, this gets interesting: it looks like we try to do the graceful shutdown in the Drop impl for the CheckWatcher.
I have no clue why does not it finish correctly though, since don't know how it interacts with VS Code.
Might it be that we need to send some response to VS Code on termination also?

eriktrom · 2020-03-25T01:20:56Z

@SomeoneToIgnore - never used vscode framework, but give this a shot, in editors/code/src/config.ts, line 77, return Promise.resolve() so that when u pass the callback to the constructor function's first ev listener(in that file), this.refreshConfig() is always scheduled in the microtask queue where promises resolve. U should then also be able to remove this.refreshConfig(); from the 2nd line in the constructor, thus forcing it to be called in the same event loop stack frame.

I've noticed this bug, so if you need help, holler, don't mind helping out.

(and of course, i could be way wrong and the scheduling could very well be benign)

Veetaha · 2020-03-25T02:10:44Z

@eriktrom what bug are you talking about? What do you mean by passing the callback to the event listener? Maybe you meant registering the event listener callback which is this line:
https://github.com/rust-analyzer/rust-analyzer/blob/6ad1a0711631d8017791a6dfe85bbe205d6c7414/editors/code/src/config.ts#L32
And I don't see any bugs and how microtasks should be connected with them here.

@SomeoneToIgnore if you feel that there is something that is broken on the frontend that doesn't support the reloading, you can pass this task to me. Anyway, my guess is that the server doesn't send the end event when it interrupts the cargo watcher. So this line here ends with an error
https://github.com/rust-analyzer/rust-analyzer/blob/6ad1a0711631d8017791a6dfe85bbe205d6c7414/crates/ra_cargo_watch/src/lib.rs#L381

because the communication channel was closed abruptly in the WatchThread::drop()
https://github.com/rust-analyzer/rust-analyzer/blob/6ad1a0711631d8017791a6dfe85bbe205d6c7414/crates/ra_cargo_watch/src/lib.rs#L398

From my perspective, it looks like the arr two solutions:

Add a Shutdown command here:
https://github.com/rust-analyzer/rust-analyzer/blob/6ad1a0711631d8017791a6dfe85bbe205d6c7414/crates/ra_cargo_watch/src/lib.rs#L98-L101

And properly handle it here by sending the WorkDoneProgressEnd task:
https://github.com/rust-analyzer/rust-analyzer/blob/6ad1a0711631d8017791a6dfe85bbe205d6c7414/crates/ra_cargo_watch/src/lib.rs#L123-L129

And send the Shutdown command in drop().

Don't drop the cargo watcher, but maybe instead just stop the current check process by sending the new Update command which will also reastart cargo with the new config. However, we need to add some API to update the watcher config. Maybe the Update command should supply the options themselves (i.e. make Update command a tuple enum variant that includes CheckOptions)?

Looks like it is very intuitive to do it the second way. What do you think @kiljacken @matklad @SomeoneToIgnore

eriktrom · 2020-03-25T03:32:38Z

sorry let me clarify succinctly:

fe379be#diff-519236f72123ff858d99b50e506bf6ccL56-L57

fe379be#diff-519236f72123ff858d99b50e506bf6ccL70-L87

in the constructor. the callback this.onConfigChange, registered on line 56 by the vscode.workspace.onDidChangeConfiguration(this.onConfigChange, this, ctx.subscriptions); event handler is async. But when called, due to being an async callback, we're already in a different turn of the event loop. So line 57 is being called immediately, and the method this.onConfigChange is non-deterministically called by the event handler (registered on line 56), I'd need to debug with a breakpoint but a few things can go wrong, the instance could be destroyed in that time period, we might then re-register that async callback if its a hot path, preventing the normal sync stack from flushing, (b/c promises block while they're queue has any microtasks), leading to a timeout, that then requires an host extension restart.

I'm not sure how vscode does its internal scheduling, so I just commented offhand as though it didn't and therefore, take with grain of salt.

BTW - I love rust-analyzer :) thanks so much for ur work.

editors/code/src/config.ts

kiljacken · 2020-03-25T09:13:05Z

Anyway, my guess is that the server doesn't send the end event when it interrupts the cargo watcher. So this line here ends with an error

https://github.com/rust-analyzer/rust-analyzer/blob/6ad1a0711631d8017791a6dfe85bbe205d6c7414/crates/ra_cargo_watch/src/lib.rs#L381

because the communication channel was closed abruptly in the WatchThread::drop()

https://github.com/rust-analyzer/rust-analyzer/blob/6ad1a0711631d8017791a6dfe85bbe205d6c7414/crates/ra_cargo_watch/src/lib.rs#L398

I believe your analysis is correct. I didn't really consider cancelling the check thread without starting a new run immediately afterwards which masks the underlying issue. I believe we are robust against multiple end messages in the extension, so just making sure we send it in a shutdown command or something else should be fine.

edwin0cheng · 2020-03-25T10:38:56Z

And properly handle it here by sending the WorkDoneProgressEnd task:

@Veetaha @SomeoneToIgnore One thing I want to clarify here is that the Cargo progress bar is a custom progress bar, not the default VSCode progress bar.

matklad · 2020-03-28T08:39:04Z

editors/code/src/main.ts

@@ -93,6 +93,12 @@ export async function activate(context: vscode.ExtensionContext) {
        activateHighlighting(ctx);
    }
    activateInlayHints(ctx);
+
+    vscode.workspace.onDidChangeConfiguration(
+        _ => ctx?.client?.sendNotification('workspace/didChangeConfiguration', { settings: "" }),


Hm, is there perhaps some way to make the client library we use do this automatically?

matklad · 2020-03-28T08:39:47Z

editors/code/src/ctx.ts

@@ -20,6 +20,7 @@ export class Ctx {
        const res = new Ctx(config, extCtx, client, serverPath);
        res.pushCleanup(client.start());
        await client.onReady();
+        client.onRequest('workspace/configuration', _ => [configToServerOptions(config)]);


Similarly, is there some kind of build-in handler here, which just fetches the relevant section from the config?

matklad · 2020-03-28T08:40:31Z

crates/rust-analyzer/src/main_loop.rs

@@ -624,7 +661,17 @@ fn on_notification(
        Err(not) => not,
    };
    let not = match notification_cast::<req::DidChangeConfiguration>(not) {
-        Ok(_params) => {


Hm, yeah, given that we already have no-op handler to avoid errors, I think clients do send this notification already?

It did not trigger for me when I tried to change the settings on master, so it looks like they don't.

But I find this surprising also, I would have expected it to be working by default.

Co-Authored-By: Veetaha <veetaha2@gmail.com>

SomeoneToIgnore · 2020-03-30T11:51:18Z

I've finally found some time to dig and fix the progress bar issue.

We cannot use the way discussed at #3666 (comment) due to the way the main loop works: it uses the task_recv from the current check watcher.
This means that we cannot send whatever message from the old watcher and just drop it: the message won't be received by anybody, hence no progress bar cleaning will be done ever.

Instead, I clean up all diagnostics and stop the progress bad on CheckWatcherThread::run which is done once when the new watcher starts.

With this fixed, I've addressed all feedback up to here.

The last portion of the feedback remaining is the one from @matklad about the automatic config requests/responses handling, the one that I've outlined in the PR header as

if there's a better way to handle those responses, let me know.

I still have no idea if there's a better way and will gladly put a fixme and fix it later instead of dealing with it now and holding the PR for whatever uncertain amount of time more.

matklad · 2020-03-30T12:45:33Z

bors r+

bors · 2020-03-30T12:52:33Z

Build succeeded

SomeoneToIgnore changed the title ~~Reload configuration~~ Reload part of the server configuration without restarts Mar 20, 2020

edwin0cheng reviewed Mar 21, 2020

View reviewed changes

crates/rust-analyzer/src/world.rs Outdated Show resolved Hide resolved

Veetaha reviewed Mar 21, 2020

View reviewed changes

crates/rust-analyzer/src/main_loop.rs Outdated Show resolved Hide resolved

Veetaha approved these changes Mar 21, 2020

View reviewed changes

SomeoneToIgnore force-pushed the reload-configuration branch from d7283b3 to 7b9187c Compare March 21, 2020 22:49

kiljacken reviewed Mar 22, 2020

View reviewed changes

crates/rust-analyzer/src/world.rs Show resolved Hide resolved

SomeoneToIgnore force-pushed the reload-configuration branch 2 times, most recently from a327c27 to 3918ef3 Compare March 22, 2020 16:26

Veetaha reviewed Mar 23, 2020

View reviewed changes

crates/rust-analyzer/src/main_loop.rs Outdated Show resolved Hide resolved

edwin0cheng reviewed Mar 24, 2020

View reviewed changes

crates/rust-analyzer/src/main_loop.rs Outdated Show resolved Hide resolved

matklad reviewed Mar 24, 2020

View reviewed changes

crates/rust-analyzer/src/main_loop.rs Outdated Show resolved Hide resolved

eriktrom reviewed Mar 25, 2020

View reviewed changes

editors/code/src/config.ts Outdated Show resolved Hide resolved

SomeoneToIgnore mentioned this pull request Mar 28, 2020

Way to toggle inlay hints without reload #3751

Closed

matklad reviewed Mar 28, 2020

View reviewed changes

SomeoneToIgnore force-pushed the reload-configuration branch from fe379be to e21d734 Compare March 30, 2020 10:16

SomeoneToIgnore added 4 commits March 30, 2020 13:39

Client side draft

8c4aab0

Process configuration response draft

019f269

Send the config from the client

a9dd442

Read new config on the server side

2feaef9

SomeoneToIgnore and others added 9 commits March 30, 2020 13:39

Reload only the properties that do not affect vfs

332799d

Style fixes

8a23bec

Reload watcher configuration

bbe59c7

Remove an obsolete fixme

392eb74

Small style fix

590af37

Co-Authored-By: Veetaha <veetaha2@gmail.com>

Code review fixes

b892a48

Co-Authored-By: Veetaha <veetaha2@gmail.com>

Small style fix

fbef012

Avoid failing on incorrect settings response

2a19459

Rebase fixes

8e78371

SomeoneToIgnore force-pushed the reload-configuration branch from e21d734 to 8e78371 Compare March 30, 2020 11:15

Fix the endless progress bar issue

4c897d8

bors bot merged commit 671926a into rust-lang:master Mar 30, 2020

SomeoneToIgnore deleted the reload-configuration branch March 30, 2020 12:53

SomeoneToIgnore mentioned this pull request Apr 2, 2020

Cargo check is not disabled #3823

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reload part of the server configuration without restarts #3666

Reload part of the server configuration without restarts #3666

SomeoneToIgnore commented Mar 20, 2020 •

edited

Loading

Veetaha left a comment

SomeoneToIgnore commented Mar 23, 2020

Veetaha commented Mar 23, 2020 •

edited

Loading

edwin0cheng commented Mar 24, 2020 •

edited

Loading

matklad commented Mar 24, 2020 via email

edwin0cheng commented Mar 24, 2020 •

edited

Loading

SomeoneToIgnore commented Mar 24, 2020

eriktrom commented Mar 25, 2020

Veetaha commented Mar 25, 2020 •

edited

Loading

eriktrom commented Mar 25, 2020 •

edited

Loading

kiljacken commented Mar 25, 2020

edwin0cheng commented Mar 25, 2020

matklad Mar 28, 2020

matklad Mar 28, 2020

matklad Mar 28, 2020

SomeoneToIgnore Mar 28, 2020

SomeoneToIgnore commented Mar 30, 2020

matklad commented Mar 30, 2020

bors bot commented Mar 30, 2020

Reload part of the server configuration without restarts #3666

Reload part of the server configuration without restarts #3666

Conversation

SomeoneToIgnore commented Mar 20, 2020 • edited Loading

Veetaha left a comment

Choose a reason for hiding this comment

SomeoneToIgnore commented Mar 23, 2020

Veetaha commented Mar 23, 2020 • edited Loading

edwin0cheng commented Mar 24, 2020 • edited Loading

matklad commented Mar 24, 2020 via email

edwin0cheng commented Mar 24, 2020 • edited Loading

SomeoneToIgnore commented Mar 24, 2020

eriktrom commented Mar 25, 2020

Veetaha commented Mar 25, 2020 • edited Loading

eriktrom commented Mar 25, 2020 • edited Loading

kiljacken commented Mar 25, 2020

edwin0cheng commented Mar 25, 2020

matklad Mar 28, 2020

Choose a reason for hiding this comment

matklad Mar 28, 2020

Choose a reason for hiding this comment

matklad Mar 28, 2020

Choose a reason for hiding this comment

SomeoneToIgnore Mar 28, 2020

Choose a reason for hiding this comment

SomeoneToIgnore commented Mar 30, 2020

matklad commented Mar 30, 2020

bors bot commented Mar 30, 2020

Build succeeded

SomeoneToIgnore commented Mar 20, 2020 •

edited

Loading

Veetaha commented Mar 23, 2020 •

edited

Loading

edwin0cheng commented Mar 24, 2020 •

edited

Loading

edwin0cheng commented Mar 24, 2020 •

edited

Loading

Veetaha commented Mar 25, 2020 •

edited

Loading

eriktrom commented Mar 25, 2020 •

edited

Loading