support canceling pending completions request

[rchl]

Adds support for canceling pending completions requests when ST triggers on_query_completions handler while one is currently in progress.

Created QueryCompletionsTask (not one of SaveTask's, the naming similarity is just coincidental) class that encapsulates the textDocument/completion requests and makes it possible to cancel them thanks to having enough context.

Canceling the completion task means that:

• any pending textDocument/completion requests are canceled with $/cancelRequest.
• the sublime completion list is immediately resolved with empty completions
• since the server might or might not support canceling of completion requests, it will either still return with a full response later, with an empty response or with an error response. In all cases we just ignore the responses and avoid processing potentially very big payload.

This can improve typing experience dramatically if the server is slow and especially if the responses are also very big (50MB is not unheard of in Typescript). Previously it was possible to trigger multiple requests one after each other which would queue and get processed as they come, resulting in a big UI lag. With this change only the latest response will be processed.

I will also add support for canceling completion request to typescript-language-server which will further help this case.

[predragnikolic]

First call hierarchy, then this! 🙂

Rafal can you suggest a server where this can be tested?

[rchl]

This name is kinda silly but I needed a variant that returns request_id and naming is hard.
The alternative could be to pass request_id "by reference" through an optional argument but I've chosen this approach in the end.

[predragnikolic]

Currently the request_id is stored on the session,
but if would store the id on the Request class the send_request_task_2 could be removed.
I did a quick experiment here -> d266190

[rchl]

I did think about it like that but implementing it without hacks seemed like too much work. I don't like the approach with a global. Globals are rarely a correct way to do things. Also it's not thread-safe since we are creating Request instances both on the main and async thread.

[predragnikolic]

You know what, this function is not that bad.
It is the same as the above function, not that complicated, and the only difference is the return type.

If you find a way to remove it, good, if not, it still ok.

[rchl]

First call hierarchy, then this! 🙂

Rafal can you suggest a server where this can be tested?

I'm adding support for canceling in typescript-language-server in typescript-language-server/typescript-language-server#672 but server supporting cancellation is not really a requirement for testing it. The only difference when server supports canceling is that it will return faster and likely return an empty response (or an error response).

For now you could test in LSP-volar or LSP-typescript in some context where completion request is slow and returns a lot of completions. In that case if you quickly type let's say "x " (where the second character is a space) then you should trigger two completion requests and the first one should get canceled. For example this is a log of such action:

For testing it would also be useful to log the len(completions) in _on_query_completions_resolved_async to verify that the list is empty for the canceled queries.

[rchl]

Note that there is open core issue for the fact that ST doesn't re-trigger on_query_completions when typing punctuation characters while there is a pending completion. So in that case we are not able to cancel. For example when typing a.a.a.a.a quickly the completions will be triggered for the initial character only and not canceled on typing . which is wrong.

[predragnikolic]

Sorry for the late reply,

Here are just my investigation:

Some things I have read about the spec

Here is what the spec says:

// method: ‘$/cancelRequest’

interface CancelParams {
	/**
	 * The request id to cancel.
	 */
	id: integer | string;
}

A request that got canceled still needs to return from the server and send a response back. It can not be left open / hanging. This is in line with the JSON-RPC protocol that requires that every request sends a response back. In addition it allows for returning partial results on cancel. If the request returns an error response on cancellation it is advised to set the error code to ErrorCodes.RequestCancelled.

Right now, the current PR:

any pending textDocument/completion requests are canceled with $/cancelRequest.

This is ok.

the sublime completion list is immediately resolved with empty completions.
Since the server might or might not support canceling of completion requests, it will either still return with a full response later, with an empty response or with an error response. In all cases we just ignore the responses and avoid processing potentially very big payload.

If the server does support canceling, it can return partial results.
So we choose between:
a) showing partial results, but have slow experience with servers that do not support canceling,
b) immediately resolving completions, but having the same fast experience even with servers that do not support canceling.

For me the b) option is better.

I like the PR
and I also have a feeling that we can cancel all pending requests for any method.

The current PR handles only completions,
But I have a feeling that we can cover all requests.

I have an idea but haven't tried it.

Lets look at the Session(LSP/plugin/core/sessions.py):

class Session(TransportCallbacks):
	def __init__(...):
		...
+		self.pending_requests = {
+			# key: method_name, value: request_id
+		}
    def send_request_async(self, request: Request,...):
+       # add code to cancel tokens
+       pending_id = self.pending_requests.get(request.method
+		if pending_id:
+			send_cancel_request(pending_id)
        self.request_id += 1
+       self.pending_requests[request.method] = self.request_id
        request_id = self.request_id
        if request.progress and isinstance(request.params, dict):
            request.params["workDoneToken"] = _WORK_DONE_PROGRESS_PREFIX + str(request_id)

    def deduce_payload(self, payload...):
        if "method" in payload:
            method = payload["method"]
            req_id = payload["id"]
+           if req_id == self.pending_requests.get(method):
+	 	       self.pending_requests[method] = None

If we known that a request that got canceled still needs to return from the server and send a response back,
This POC code above will be enough implement cancel token for all requests, if the server implements cancellation.

Would you consider such approach?

Also note, I would not do any changes that I wrote in this PR,
If I have time I would open a new draft PR
just to compare this PR with canceling pending requests in the session
But I would not do it if you see some flows with it.

[rchl]

If the server does support canceling, it can return partial results. So we choose between: a) showing partial results, but have slow experience with servers that do not support canceling, b) immediately resolving completions, but having the same fast experience even with servers that do not support canceling.

For me the b) option is better.

IMO partial results don't apply in this case because we are only canceling completion requests if another one was made so the results from the canceled one don't apply anymore.

(Also ST wouldn't even show results for that completion list since it already made new completions query)

If we known that a request that got canceled still needs to return from the server and send a response back,
This POC code above will be enough implement cancel token for all requests, if the server implements cancellation.

Just using the request method as a way of telling whether request can be canceled is not enough since there can be multiple requests with the same method name but made for different files, for example. In which case we'd need to key by method+uri. And the logic might need to be request-specific to know what to key on.

Certainly out of scope of this PR anyway.