avoid overloading ls with completion resolve requests #10539
Conversation
pascalkuthe
added
C-bug
pascalkuthe
force-pushed
the
completion_resolve_dos
branch
from
a4dc696
to
8610a0e
Compare
| pub tags: Vec<DiagnosticTag>, | ||
| pub source: Option<String>, | ||
| pub data: Option<serde_json::Value>, | ||
| } | ||
|
|
||
| // TODO turn this into an enum + feature flag when lsp becomes optional | ||
| pub type DiagnosticProvider = LanguageServerId; |
There was a problem hiding this comment.
👍 this will be useful when introducing spell checking diagnostics
|
|
||
| let res = self.call::<lsp::request::ResolveCompletionItem>(completion_item); | ||
| Some(async move { Ok(serde_json::from_value(res.await?)?) }) | ||
| completion_item: &lsp::CompletionItem, |
There was a problem hiding this comment.
Ah nice, I remember thinking there was an unnecessary clone in completion resolving because of the type here lacking a ref 👍
pascalkuthe
force-pushed
the
completion_resolve_dos
branch
from
0f5c028
to
61d2db6
Compare
There was a problem hiding this comment.
Nice work! I think I've seen the completion resolve bug cause some odd effects in ErlangLS (which is relatively rather slow).
Both the refactor towards a slotmap for language servers and moving the completion resolve handler to its own file are 👍. The diff is a little large but I think that's a good sign that we should have a more formal ID for language servers anyways.
pascalkuthe
force-pushed
the
completion_resolve_dos
branch
from
61d2db6
to
d533207
Compare
|
Agreed, the changes look good! Do we also need a newtype ID for debuggers? Just one nit, can you copy the problem description/solution part of the PR description into the last commit message? It wasn't clear to me what "don't resolve lsp with completion resolve" meant until I went back and read the description |
While moving completion resolve to the event system in #9668 we introduced what is essentially a "DOS attack" on slow LSPs. Completion resolve requests were made in the render loop and debounced with a timeout. Once the timeout expired the resolve request was made. The problem is the next frame would immediately request a new completion resolve request (and mark the old one as obsolete but because LSP has no notion of cancelation the server would still process it). So we were in essence sending one completion request to the server every 150ms and only stopped if the server managed to respond before we rendered a new frame. This caused overload on slower machines/with slower LS. In this PR I revamped the resolve handler so that a request is only ever resolved once. Both by checking if a request is already in-flight and by marking failed resolve requests as resolved.
pascalkuthe
force-pushed
the
completion_resolve_dos
branch
from
d533207
to
5f070f8
Compare
I amended the commit message. Adding newtype ids for DAP would be nice too (in general I almost always prefer newtyped integers to raw integers for ids) but I would defer that to a different PR. |
While we don't have any reports about this on github I saw a bunch of people having issues with crashing/overloaded lsps on reddit/matrix in the latest release. While working on larger improvements to the completion system I noticed the root cause for that.
While moving completion resolve to the event system in #9668 we introduced what is essentially a "DOS attack" on slow LSPs. Completion resolve requests were made in the render loop and debounced with a timeout. Once the timeout expired the resolve request was made. The problem is the next frame would immediately request a new completion resolve request (and mark the old one as obsolete but because lsp has no nothing of cancelation the server would still process it). So we were in essence sending one completion request to the server every 150ms and only spotted if the server managed to respond before we rendered a new frame. This caused overload on slower machines/with slower lsp.
In this PR I revamped the resolve handler so that a request is only ever resolved once. Both by checking if a request is already inflight and by marking failed resolve requests as resolved.
This PR was split off on larger changes to the completions since this is a somewhat major regression in the last release that I want to fix ASAP. To avoid a huge amount of merge conflicts on my side I kept one additional commit from the larger changes (that the resolvers changes were based on). That commit replaces the
usizewe used for the language server id with a proper type (using slotmap). That also turns looking up a language server by id/by name fromO(n)toO(1)operations.