Testing `textDocument/didClose` in extensions requires a three minute timeout

[symmb]

When opening a file in tests using vscode.workspace.openTextDocument followed by vscode.window.showTextDocument and closing it again using workbench.action.closeAllEditors or workbench.action.closeActiveEditor, a textDocument/didClose notification is not fired until three minutes have passed since the file was opened. I assume this happens for the same reason as the three minute timeout in #652.

While it's possible to test functionality that depends on a didClose notification with a three minute pause, it's not ideal. Is there a way to open (or close) files in extension tests that fires the notification sooner?

In case it helps, I've also put together a small reproducer. Just compare the behavior of manually opening foo.txt in the file tree and closing it after the language server has started with what the test does.

Tested on VS Code version 1.70.2.

[symmb]

What's even more bizarre: We've implemented our own notification for detecting when files are closed and those are fired immediately in tests. I took a look at the implementation of didClose in the language client and, as far as I could tell, it's pretty much the same as our custom notification.

Here's the code responsible for our notification, if it helps.

```ts context.subscriptions.push( vscode.window.tabGroups.onDidChangeTabs(async (e) => { const _client = client; // "client" holds an instance of LanguageClient or null. if (!_client) { return; } function urisFromTabs(tabs: readonly vscode.Tab[]): string[] { return tabs .filter((tab) => tab.input instanceof vscode.TabInputText) .map((tab) => _client!.code2ProtocolConverter.asUri((tab.input as vscode.TabInputText).uri)); } const closedURIs = urisFromTabs(e.closed); if (closedURIs.length > 0) { const openTabs = vscode.window.tabGroups.all.map((tg) => tg.tabs).reduce((a, b) => [...a, ...b], []); const openURIs = new Set(urisFromTabs(openTabs)); const reallyClosedURIs = closedURIs.filter((uri) => !openURIs.has(uri)); const parameters = { files: reallyClosedURIs.map((uri) => ({ uri, })), }; await _client.sendNotification('symflower/filesClosed', parameters); } }), ); ```

[dbaeumer]

In LSP there is a big conceptual difference between a editor closing and a document closing. The document open/ close event is a ownership transfer between client and server. A server can't touch an open document on disk. Since VS Code keeps the file open the LSP client can't make the file as closed since VS Code could (for example through another operation) change its content in memory bring client and server out of sync.

We could look into some improvements in the Lib to filter out open calls for documents that are not visible however that needs some careful thinking since we need to ensure that we don't get out of sync (we would need to send a fake open in case VS Code touches the document).

[symmb]

I see, so we shouldn't rely on open and close notifications corresponding to any user-facing event in the client because that's not what they represent. Sorry if I've misread the docs on that. Is there any standard way in LSP to listen for "visible" documents then (whatever that means for a client) or does that require custom extensions?

The suggestion of filtering calls sounds good although we don't currently need it if didClose isn't the right tool for our use case.

[dbaeumer]

No, there is no LSP API to know what is visible in the UI. This is on purpose since LSP specs it clearly that requests are valid for all kinds of documents (e.g. closed once and once that are open and not visible in the UI).

We only saw one use case where not knowing this caused problems and this was pushing document diagnostics. This is why LSP now has a pull model where the client can priorities pulls based on UI state.

[symmb]

Okay, for clarification, our specific use case is only displaying diagnostics for files that are (from a user's POV) open in an editor. I've just checked how other language servers that do something similar implement this and it seems that textDocument/didClose is often used for updating diagnostics in this case (also for example in VS Code's own JSON server).

Does that mean that, when relying on didClose, this behavior is more or less incidental and can't reliably be tested using a VS Code extension test? (Of course it can still be tested when communicating directly with the language server since the test can send any notification it wants in that setup.)

[dbaeumer]

didOpen and didClose fire when an editor (e.g. VS code) reads the content of a file into a buffer. They fire even if the buffer is not presented in a editor. For example if you run a rename refactoring VS Code loads lots of buffers into memory to apply the text edits. This case open/close events even though these documents are not visible in the UI.

The pull diagnostic feature doesn't pull for diagnostics for documents that are not visible in the UI assuming that the user is not of interest for them.

Pull diagnostics is a relatively new feature and we started to port our servers over to that feature.

[DanTup]

We only saw one use case where not knowing this caused problems and this was pushing document diagnostics.

Not knowing the open files has caused issues outside of this for me too. In our server we prioritise open files (it's a single-threaded async server) but they're usually polluted with older open files. VS Code also sends an open/close request right before people Ctrl+Click symbols (because holding Ctrl when hovering shows a preview) which causes the server to start doing some work, which then becomes immediately unnecessary, but blocks the following Go-to-Definition until it completes. (I can't come up with an explanation for how this immediate open/close is useful, but that's another issue 🙂).

I don't know why VS Code couldn't have an API for testing that flushes all of these close events. Testing VS Code extensions has always been quite a frustrating endeavour (I suspect there's some chicken/egg in not many people doing it, so it's not worth spending time on better APIs, the lack of which is why not many people do it).

[symmb]

@dbaeumer Thanks for pointing me at pull diagnostics! That seems to be what I was looking for. I still have one more question but since it's more generally about LSP and to prevent this discussion from getting out of hand I've opened a new issue at https://github.com/microsoft/language-server-protocol/issues/1542

[dbaeumer]

Regarding testing: the LSP client offers API to directly talk to a server which I extensively use for testing. If you in tests avoid opening the references view there should be no timeout necessary for didClose. Look her for how you can use the LSP client directly to talk to a server without going through VS Code API: https://github.com/microsoft/vscode-languageserver-node/blob/8de18faed635819dd2bc631d2c26ce4a18f7cf4a/client-node-tests/src/integration.test.ts#L351

[DanTup]

@dbaeumer thanks - I didn't provide a lot of context. My tests are mostly integration tests in VS Code (to ensure things are working as expected.. I've hit many issues in the past that would have not been caught by testing the providers/server but were down to how VS Code interpreted the data is was given).

Testing that a priority file is removed after a document is closed in VS Code, or just ensuring all tests start with a blank slate (without having every test spawn in its own workspace - something that can't be controlled from inside test code) is not feasible. It's not really an LSP thing, that's really a VS Code gripe. The thing that affected my in LSP was really the unwanted open/close events that appear to serve zero purpose to LSP servers (but can make a servers single thread busy for a period, delaying other requests - unless we buffer open events to see if they're immediately closed after - which can add a delay to starting to process them and is an odd workaround inside an LSP server to account for unwanted behaviour of a single client).

[dbaeumer]

I have ideas how to fold these open/close events however we need to ensure that this code handles all cases right (e.g. when a open non visible document gets modified we need to inform the server about it). Otherwise we will see strange errors since data will get out of sync.

Regarding the three minute timeout: have you opened an issue against VS Code itself and explained the problem with testing. The underlying problem seems to exist outside of LSP as well.

[DanTup]

I have ideas how to fold these open/close events however we need to ensure that this code handles all cases right

It'd be great to fix here, but it's disappointing that it needs working around here. I don't understand why VS Code doesn't handle this better (eg. let an extension know if it's opening a file only to read the contents and immediately close it for its own use - like populating the hover - where an extension/server very likely does not care about).

Regarding the three minute timeout: have you opened an issue against VS Code itself and explained the problem with testing. The underlying problem seems to exist outside of LSP as well.

Yeah, I've filed many issues like this about things being hard to test in VS Code. Usually the response is that I shouldn't be integration testing, and should just test providers etc. individually (although I disagree with this, because my integration tests have caught many issues - both my own and VS Code's that testing the providers would not have caught). For this particular issue, all issues about it were closed as dupes of https://github.com/microsoft/vscode/issues/15178 (which helps for some cases, although not really for getting a clean state for integration tests without new workspaces).

[EhabY]

For future reference, a good workaround for didClose notifications to be sent when an editor is closed is to use:

await vscode.commands.executeCommand("vscode.open", uri, options)

Instead of

const document = await vscode.workspace.openTextDocument(uri);
await vscode.window.showTextDocument(document, options);

See Built-in Commands and TextDocumentShowOptions

It seems when VS Code handles the opening and closing, notifications are sent as expected.