microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.31k stars 28.9k forks source link

Enable to cancel the request in workspace.onDidChangeTextDocument #123962

Closed jdneo closed 3 years ago

jdneo commented 3 years ago

The new test API requires to leverage workspace.onDidChangeTextDocument to watch the changes to a text document.

In Java, we previously use CodeLens provider to provide shortcuts for running and debugging tests. And we leverage the cancellation token in the Code Lens provider to early return when an edit action is cancelled. This is to make sure the performance of the core edit features (e.g. completion) will not be impacted too much. Can the cancellation token also be considering in workspace.onDidChangeTextDocument? @connor4312

jdneo commented 3 years ago

Another request is about the adaptive debounce. @jrieken As we discussed in #106267, Will VS Code team consider to make it reusable for other components?

egamma commented 3 years ago

//CC @jrieken regarding the ping for a node module for debouncing events.

Will VS Code team consider to make it reusable for other components?

connor4312 commented 3 years ago

Cancellation is not something we would have an event. The process of cancellation, in a Provider for instance, is used to signal that the result of the method call is no longer relevant or the editor is no longer interested in it. Events are "fire and forget". The editor does not assume any particular lifecycle in the event handlers and is not interested in any results/effects those handlers might have.

If you want to, for example, stop a background process triggered by an onDidChangeTextDocument whenever another event comes in for the same document or that text document closes, that's something you will need to deal with manually -- for example, by keeping a Map<TextDocument, IDisposable>.

Will let Joh respond to the adaptive debounce query

jdneo commented 3 years ago

@connor4312 Thank you for the explanation.

In Java, the cancellation token can be passed via LSP4J to the server side and use it to cancel the request, which is a quite nature and neat way to early return to request. Though, we might make some variables which can be shared across multi threads and achieve the same effect, that's far more complex compared with the usage of cancellation token.

If workspace.onDidChangeTextDocument is not the place to add the cancellation token. Can it be added to other related APIs, like the token currently used in createDocumentTestRoot()?

I really like the UI of the left side bar shortcut and hope to use it replace the current usage of Code Lenses. Hope this can be taken into consideration.

Thanks!

connor4312 commented 3 years ago

Ah, I see the problem. You end up needing to 'debounce' the change events, and potentially cancel yourself. I think that this is still something that the extension should deal with manually based off the change event, but maybe there is a recommended approach or small library we could use to make solving the problem easier.

jdneo commented 3 years ago

Yes, because previously we have some performance issue with the code completion. To improve that, we use the two approaches to boost the resolving time: debounce to reduce the number of request and cancellation token to early return.

jrieken commented 3 years ago

Another request is about the adaptive debounce. @jrieken As we discussed in #106267, Will VS Code team consider to make it reusable for other components?

Doing that comes at an extra cost for us and since we would not consume said module that cost isn't paying off. I would recommend the pragmatic approach of just copying bits from our sources. In fact, we don't even have the adopting event debouncing, we only have some adapting request delaying for things like code lens. Anyways, you should copy these "building blocks"