Agent: add cancelation support #787
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
olafurpg
commented
Aug 22, 2023
| @@ -336,6 +337,21 @@ export class Position implements VSCodePosition { | |||
| } | |||
| } | |||
|
|
|||
| export class Location implements VSCodeLocation { | |||
There was a problem hiding this comment.
This change is unrelated to cancelation. There was a new esbuild linking error from new graph/codeintel related changes so I fixed it here while working on cancelation.
| export class CancellationTokenSource { | ||
| public token: unknown | ||
|
|
||
| export class CancellationToken implements vscode_types.CancellationToken { |
There was a problem hiding this comment.
I'm not sure I managed to mirror the behavior correctly here. Would be good to get a second pair of eyes on the implementation.
olafurpg
added a commit
to sourcegraph/sourcegraph-public-snapshot
that referenced
this pull request
Aug 22, 2023
Fixes https://github.com/sourcegraph/sourcegraph/issues/56042 Previously, we didn't correctly propagate autocomplete cancelation to the agent. This was problematic because we could end up queueing a lot of autocomplete requests making the experience lag. This PR fixes the problem by sending a `$/cancelRequest` notification to the agent as implemented in sourcegraph/cody#787
olafurpg
force-pushed
the
olafurpg/cancel
branch
from
8e93f4b
to
3efc372
Compare
vdavid
reviewed
Aug 22, 2023
| @@ -162,13 +162,12 @@ export class Agent extends MessageHandler { | |||
| }) | |||
| return null | |||
| }) | |||
| this.registerRequest('autocomplete/execute', async params => { | |||
| this.registerRequest('autocomplete/execute', async (params, token) => { | |||
There was a problem hiding this comment.
Nit: Not sure about best practices, but I find the word "token" overused in programming in general. (Think access tokens, refresh tokens, bearer tokens, cancellation tokens, etc.) As a result, I usually err on the side of being specific in every case; I'd probably call this one "cancellationToken", but "cancelToken" is also fine, to match the rest of the code. WDYT? (I know you just moved this from a local variable, but it might be a good time for a rename.)
There was a problem hiding this comment.
I agree in the general case but I think it's fine to use token here since we're wrapping that vscode API that uses token
/**
* Provides inline completion items for the given position and document.
* If inline completions are enabled, this method will be called whenever the user stopped typing.
* It will also be called when the user explicitly triggers inline completions or explicitly asks for the next or previous inline completion.
* In that case, all available inline completions should be returned.
* `context.triggerKind` can be used to distinguish between these scenarios.
*
* @param document The document inline completions are requested for.
* @param position The position inline completions are requested for.
* @param context A context object with additional information.
* @param token A cancellation token.
* @return An array of completion items or a thenable that resolves to an array of completion items.
*/
provideInlineCompletionItems(document: TextDocument, position: Position, context: InlineCompletionContext, token: CancellationToken): ProviderResult<InlineCompletionItem[] | InlineCompletionList>;
olafurpg
added a commit
to sourcegraph/sourcegraph-public-snapshot
that referenced
this pull request
Aug 22, 2023
* JetBrains: add cancelation support Fixes https://github.com/sourcegraph/sourcegraph/issues/56042 Previously, we didn't correctly propagate autocomplete cancelation to the agent. This was problematic because we could end up queueing a lot of autocomplete requests making the experience lag. This PR fixes the problem by sending a `$/cancelRequest` notification to the agent as implemented in sourcegraph/cody#787 * Document cancelation * Dispose cancelation token to avoid redundant requests Previously, we sent `$/cancelRequest` for every completion including successful ones.
Previously, the agent didn't support canceling autocomplete requests. This PR adds support for cancellation through the new `$/cancelRequest` notification, which mirrors the structure of the same notification in LSP.
olafurpg
force-pushed
the
olafurpg/cancel
branch
from
3efc372
to
9a493a4
Compare
vdavid
approved these changes
Aug 23, 2023
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, the agent didn't support canceling autocomplete requests. This PR adds support for cancellation through the new
$/cancelRequestnotification, which mirrors the structure of the same notification in LSP.Test plan
Manually tested with IntelliJ here https://github.com/sourcegraph/sourcegraph/pull/56119