fix: cancel pending requests as early as possible

[rchl]

On document change, it makes sense to cancel pending semantic tokens/pull diagnostics requests as early as possible rather than waiting for the debounce timeout and potentially failing to cancel request on time and wasting resources on handling those.

[rchl]

I have an urge to rename pending_response to pending_request.

pending_response sounds like a boolean state (waiting for response) rather than what it actually is.

[jwortmann]

My thinking was that the request has already happened (was sent), so we're indeed only still waiting for the response now. But maybe we use the word "pending" in a sligthly different meaning. I guess it can both mean something like "upcoming" (that's how I used it here), or "unanswered". Of course the server response is directly coupled to the request, so feel free to rename it if you like to.

[jwortmann]

In that case I guess we don't need these parts anymore:

LSP/plugin/session_buffer.py

Lines 489 to 492 in 04827c7

	if self._document_diagnostic_pending_request:
	if self._document_diagnostic_pending_request.version == version:
	return
	self.session.cancel_request(self._document_diagnostic_pending_request.request_id)

LSP/plugin/session_buffer.py

Lines 605 to 606 in 04827c7

	if self.semantic_tokens.pending_response:
	self.session.cancel_request(self.semantic_tokens.pending_response)

Or is there maybe an edge case that I'm missing?

[rchl]

In that case I guess we don't need these parts anymore:

LSP/plugin/session_buffer.py

Lines 489 to 492 in 04827c7

	if self._document_diagnostic_pending_request:
	if self._document_diagnostic_pending_request.version == version:
	return
	self.session.cancel_request(self._document_diagnostic_pending_request.request_id)

LSP/plugin/session_buffer.py

Lines 605 to 606 in 04827c7

	if self.semantic_tokens.pending_response:
	self.session.cancel_request(self.semantic_tokens.pending_response)

Or is there maybe an edge case that I'm missing?

I'd have to check. I thought that we still have to cancel but upon more thinking I've ruled out what I was thinking about before.

[jwortmann]

Related issue: #1386

Should we set general.staleRequestSupport.cancel client capability to true?

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#clientCapabilities

		/**
		 * Client capability that signals how the client
		 * handles stale requests (e.g. a request
		 * for which the client will not process the response
		 * anymore since the information is outdated).
		 *
		 * @since 3.17.0
		 */
		staleRequestSupport?: {
			/**
			 * The client will actively cancel the request.
			 */
			cancel: boolean;

			/**
			 * The list of requests for which the client
			 * will retry the request if it receives a
			 * response with error code `ContentModified``
			 */
			 retryOnContentModified: string[];
		}

[rchl]

I'm fine with that.

I'm not sure what's the purpose of this capability though and whether it makes a difference that we don't consistently do canceling for every feature that could benefit from it.

[rchl]

The only reference to staleRequestSupport that I found is swiftlang/sourcekit-lsp#1650

I wonder if explicitly stating that we cancel requests would mean that we would potentially miss server canceling some requests that we don't currently cancel ourselves.

[rchl]

LSP-vue (new volar version) does something that makes the cancel in do_semantic_tokens_async still have a purpose.

:: [22:17:13.766] --> LSP-vue textDocument/semanticTokens/range (3): {'textDocument': {'uri': 'file:///usr/local/workspace/lsp/lsp-log-parser/components/app-bar.vue'}, 'range': {'start': {'line': 0, 'character': 0}, 'end': {'line': 162, 'character': 0}}}
:: [22:17:13.766] --> LSP-vue textDocument/documentLink (4): {'textDocument': {'uri': 'file:///usr/local/workspace/lsp/lsp-log-parser/components/app-bar.vue'}}
:: [22:17:13.780] <-- LSP-vue client/registerCapability (0): {'registrations': [{'id': 'a57c161c-26ab-45c1-821c-653beb6c96bc', 'method': 'workspace/didChangeConfiguration', 'registerOptions': {}}]}
:: [22:17:13.780] >>> LSP-vue (0) (duration: 0ms): None
:: [22:17:13.780] <-- LSP-vue workspace/configuration (1): {'items': [{'section': 'http'}]}
:: [22:17:13.780] >>> LSP-vue (1) (duration: 0ms): [None]
:: [22:17:13.780] <-- LSP-vue workspace/configuration (2): {'items': [{'section': 'http'}]}
:: [22:17:13.781] >>> LSP-vue (2) (duration: 0ms): [None]
:: [22:17:14.750] <-- LSP-vue client/registerCapability (3): {'registrations': [{'id': '7608602c-8896-4727-a2b6-dbf4ea45dbcb', 'method': 'workspace/didChangeWatchedFiles', 'registerOptions': {'watchers': [{'globPattern': '**/*.{js,cjs,mjs,ts,cts,mts,jsx,tsx,json,vue}'}]}}]}
:: [22:17:14.752] >>> LSP-vue (3) (duration: 1ms): None
:: [22:17:14.752] <-- LSP-vue workspace/configuration (4): {'items': [{'section': 'typescript.preferences'}]}
:: [22:17:14.752] >>> LSP-vue (4) (duration: 0ms): [{'autoImportFileExcludePatterns': [], 'importModuleSpecifier': 'shortest', 'importModuleSpecifierEnding': 'auto', 'jsxAttributeCompletionStyle': 'auto', 'quoteStyle': 'auto'}]
:: [22:17:14.752] <<< LSP-vue (2) (duration: -): None
:: [22:17:14.753] <-- LSP-vue workspace/inlayHint/refresh (5): None
:: [22:17:14.753] >>> LSP-vue (5) (duration: 0ms): None
:: [22:17:14.754] <-- LSP-vue workspace/semanticTokens/refresh (6): None
:: [22:17:14.988] <<< LSP-vue (3) (duration: -): {'resultId': '1730582234987', 'data': [...]}

Summary

:: [22:17:13.766] --> LSP-vue textDocument/semanticTokens/range
:: [22:17:14.754] <-- LSP-vue workspace/semanticTokens/refresh (6): None
:: [22:17:14.988] <<< LSP-vue (3) (duration: -): {'resultId': '1730582234987', 'data': [...]}
...

[rchl]

And since with pull diagnostics the server can also trigger workspace/diagnostic/refresh, in theory the same can happen with those.

Though that made me realize that ignoring a new request when the view version hasn't changed could in theory be incorrect if the server can return different diagnostics at different point in time (because it initialized late or something). That said, it might be a rare corner case.

EDIT: added handling for that corner case.

[jwortmann]

And since with pull diagnostics the server can also trigger workspace/diagnostic/refresh, in theory the same can happen with those.

Right, good that you thought of that.

I wonder whether ignore_pending is the best name for this parameter, it's a bit difficult to understand in my opinion. Maybe something like forced or forced_update would be better?