Stream: t-compiler/rust-analyzer

Topic: Progress handler already registered


Jonas Schievink [he/him] (Dec 01 2020 at 18:11, on Zulip):

I'm getting this error occasionally: "Progress handler for token rustAnalyzer/indexing already registered"

I'm fairly sure this did not happen when I worked on that, were there any recent changes that could have caused this?

Jeremy Kolb (Dec 01 2020 at 19:02, on Zulip):

My guess is a reindexing is happening? Reading the spec I think we're doing it wrong

Jonas Schievink [he/him] (Dec 01 2020 at 19:03, on Zulip):

The code should handle this correctly, even in the presence of cancelation

Jeremy Kolb (Dec 01 2020 at 19:05, on Zulip):

Servers can also initiate progress reporting using the window/workDoneProgress/create request. This is useful if the server needs to report progress outside of a request (for example the server needs to re-index a database). The returned token can then be used to report progress using the same notifications used as for client initiated progress. A token obtained using the create request should only be used once (e.g. only one begin, many report and one end notification should be sent to it).

Jeremy Kolb (Dec 01 2020 at 19:05, on Zulip):

Reading that leads me to believe that the token has to be unique for each indexing operation

Jonas Schievink [he/him] (Dec 01 2020 at 19:05, on Zulip):

It does, but we should only run one indexing operation at once

Jonas Schievink [he/him] (Dec 01 2020 at 19:06, on Zulip):

This is why cargo check indicators are numbered when you have a workspace open, to ensure uniqueness

Jeremy Kolb (Dec 01 2020 at 19:06, on Zulip):

Okay. I thought it also ran indexing again

Jonas Schievink [he/him] (Dec 01 2020 at 19:07, on Zulip):

It is rerun on every keypress, but should get canceled first

Jeremy Kolb (Dec 01 2020 at 19:10, on Zulip):

Actually... should we only send a begin notification if the create request is successful (and maybe log an error if the request fails)?

Jeremy Kolb (Dec 01 2020 at 19:11, on Zulip):

I'm not saying that's the issue but it might help us track it down. Things could be out of order in some weird way too

Jonas Schievink [he/him] (Dec 01 2020 at 19:13, on Zulip):

huh, looking at GlobalState::report_progress, couldn't that end up sending the notification before the request has been processed or something?

Jeremy Kolb (Dec 01 2020 at 19:19, on Zulip):

I think so... I think we need to wait for the reply of the create (and that it's happy) to make sure it's good to go. It's actually a little weird to me that the client doesn't reply with a token but...

Jeremy Kolb (Dec 01 2020 at 19:34, on Zulip):

After reading the code I believe the request will be sent on the wire but you're right in that it doesn't mean the client has finished processed it. Still that wouldn't explain the error message (assuming that it's correct)

Jonas Schievink [he/him] (Dec 01 2020 at 19:36, on Zulip):

yeah, the sending order is correct

Jonas Schievink [he/him] (Dec 01 2020 at 19:36, on Zulip):

but we don't check the response before sending notifications

Jeremy Kolb (Dec 01 2020 at 19:41, on Zulip):

That's a bug. We also ignore any errors when registering capabilities for didSave

woody77 (Dec 01 2020 at 23:46, on Zulip):

Do we have an issue for this at GitHub? (I didn't see one).

I'm seeing this (and some of our devs are seeing it constantly), as well as this, which I assume is different?

thread '<unnamed>' panicked at '

Failed to lookup FN@1951..2703 in this Semantics.
Make sure to use only query nodes, derived from this instance of Semantics.
root node:   SOURCE_FILE@0..9482
known nodes: SOURCE_FILE@0..9482

', crates/hir/src/semantics.rs:556:13
woody77 (Dec 01 2020 at 23:46, on Zulip):

@Paul Faria (fyi)

Jonas Schievink [he/him] (Dec 01 2020 at 23:53, on Zulip):

Is that on the last release or nightly/master?

Jeremy Kolb (Dec 02 2020 at 13:26, on Zulip):

I'm pretty sure that's different. I haven't run across it. Can you get a better trace?

Jeremy Kolb (Dec 02 2020 at 16:24, on Zulip):

I can reliably reproduce the duplicate tokens if I use vscode's Search functionality and click on a new file to open while indexing is happening

Jeremy Kolb (Dec 02 2020 at 16:25, on Zulip):

In fact... every time I switch to a new editor reindexing starts and we try to create new tokens

Jeremy Kolb (Dec 02 2020 at 16:27, on Zulip):

Sorry that's every time I open a new document and sometimes an existing one

Jonas Schievink [he/him] (Dec 02 2020 at 16:32, on Zulip):

Ah, so it was caused by https://github.com/rust-analyzer/rust-analyzer/pull/6637

Jonas Schievink [he/him] (Dec 02 2020 at 16:33, on Zulip):

Maybe it's just missing the self.status == Status::Ready check

Jeremy Kolb (Dec 02 2020 at 16:41, on Zulip):

Maybe... if indexing is cancelled do we automatically send a WorkDoneProgressEnd to the client?

Jonas Schievink [he/him] (Dec 02 2020 at 16:45, on Zulip):

Yes

Jonas Schievink [he/him] (Dec 02 2020 at 16:46, on Zulip):

though maybe that code path should just reuse apply_document_changes

Jonas Schievink [he/him] (Dec 02 2020 at 18:47, on Zulip):

(nevermind, that doesn't make sense)

Jonas Schievink [he/him] (Dec 02 2020 at 19:17, on Zulip):

It only reliably reproduces when out_dirs/proc macro support is enabled

Jonas Schievink [he/him] (Dec 02 2020 at 19:19, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/pull/6701 fixes the problem for me, though I unfortunately still don't fully understand what was going on

Jeremy Kolb (Dec 02 2020 at 21:00, on Zulip):

I can't reproduce it after your change

Jonas Schievink [he/him] (Dec 02 2020 at 21:01, on Zulip):

nice

woody77 (Dec 02 2020 at 21:10, on Zulip):

I'm not seeing either of the above with yesterday's (today's) nightly (although that doesn't include pull/6701...

Last update: Jul 28 2021 at 04:15UTC