Ensure that the entire chat request is cancellable #433
Conversation
| @@ -54,26 +55,40 @@ export function registerChat(languageClient: LanguageClient, extensionUri: Uri, | |||
| languageClient.info(`vscode client: Received ${JSON.stringify(message)} from chat`) | |||
|
|
|||
| switch (message.command) { | |||
| case END_CHAT_REQUEST_METHOD: | |||
There was a problem hiding this comment.
i would expect this case to be already taken care of by the 'default' case, I think we can remove this specific case I see the default case doesn't send requests right now and only notifications, this comment can be ignored
| @@ -35,14 +35,9 @@ interface MessageTrigger extends TriggerContext { | |||
| followUpActions?: Set<string> | |||
| } | |||
|
|
|||
| interface StartTrigger { | |||
| triggerType?: TriggerType | |||
| hasUserSnippet?: boolean | |||
There was a problem hiding this comment.
does this mean we never logged hasUserSnippet field anywhere so we won't be logging it going forward?
There was a problem hiding this comment.
IIRC currently we are not using hasUserSnippet from the start trigger but from message trigger. (But it could be a mistake since Q would have the snippet if the first request has it.)
| this.#log('Response for conversationId:', conversationIdentifier, JSON.stringify(response.$metadata)) | ||
| } catch (err) { | ||
| if (isObject(err) && 'name' in err && err.name === 'AbortError') { | ||
| throw new CancellationError('Q api request aborted') |
There was a problem hiding this comment.
here and down below, why do we throw cancellation error instead of returning an error object like we do in other error scenarios?
There was a problem hiding this comment.
I added an upstream error handling in withLspCancellation. I guess we should probably do one of the other
|
Important I tested this change locally and it works, the changes in this PR LGTM. |
Description
The main goal of this PR is to make entire chat request cancellable. Currently, if user cancels before the response streaming starts, the request will still go through. For the fqn piece, we can use Workerpool's "cancel" feature. Otherwise, we just need to early terminate and return a
ResponseErrorwithRequestCancelledcode.I tried to make this reusable by creating a wrapper function that creates a promise for cancellation that will "race" against the core logic: the value that is resolved first will be returned.
The other two changes:
DocumentContextandTriggerContextinto a singleTriggerContextExtractoras the separation offer no benefitsTesting
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.