Use a hook for resolving completion items #9668
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously we used the IdleTimeout event to trigger LSP `completion/resolveItem` requests. We can now refactor this to use an event system hook instead and lower the timeout.
the-mikedavis
added
A-language-server
10 tasks
pascalkuthe
approved these changes
Feb 19, 2024
univerz
reviewed
Feb 21, 2024
| } else { | ||
| self.trigger = Some(item); | ||
| self.request = None; | ||
| Some(Instant::now() + Duration::from_millis(150)) |
There was a problem hiding this comment.
I understand that you don't want to introduce 10 different settings, but what about low_latency = true which will set all these annoying debounces to zero?
There was a problem hiding this comment.
I think it would be fine to have a config section dedicated to timeouts. As we tackle the items in #9629 we'll be hard-coding a lot of debounce constants. Something like:
[editor.timeout] # or maybe 'editor.debounce'
lsp.auto-complete = 250
lsp.signature-help = 120
lsp.resolve-completion-item = 150
highlight-picker-preview = 150seems fine to me but I'm not sure how @archseer feels about it. In any case we would always advise setting the constant to something non-zero, and would probably enforce that in the code
Merged
pascalkuthe
added a commit
that referenced
this pull request
Apr 22, 2024
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.
archseer
pushed a commit
that referenced
this pull request
Apr 22, 2024
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.
Vulpesx
pushed a commit
to Vulpesx/helix
that referenced
this pull request
Jun 7, 2024
While moving completion resolve to the event system in helix-editor#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.
Chirikumbrah
pushed a commit
to Chirikumbrah/helix
that referenced
this pull request
Jun 15, 2024
While moving completion resolve to the event system in helix-editor#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.
smortime
pushed a commit
to smortime/helix
that referenced
this pull request
Jul 10, 2024
While moving completion resolve to the event system in helix-editor#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we used the IdleTimeout event to trigger LSP
completion/resolveItemrequests. We can now refactor this to use an event system hook instead and lower the timeout.Connects #9629