microsoft / vscode-languageserver-node

Language server protocol implementation for VSCode. This allows implementing language services in JS/TS running on node.js
MIT License
1.41k stars 320 forks source link

FileSystemWatcher and batching #1503

Open cdietrich opened 3 days ago

cdietrich commented 3 days ago

we are using FileSystemWatcher feature of lsp to track changes of deleted files server side. in general this works perfectly fine.

but in some cases not all files arrive in the didChangeWatchedFiles event on the server side when files are deleted client side with a small pause.

(there is also no second batch)

how does the watcher/client batch the local delete events on the server? can i debug the issue locally somehow

cdietrich commented 2 days ago

ok reading the code i have found client/src/common/client.ts has notifyFileEvent i wonder what happens when a new event is pushed between the start and end of the sendNotification await in the trigger should the trigger first replace the array and then call the sendNotification?

=> i assume a

const events = client._fileEvents;
client._fileEvents = [];
await client.sendNotification(DidChangeWatchedFilesNotification.type, { changes: events });

would help

EhabY commented 2 days ago

I was able to re-produce this by copy-pasting (in the file system) with some pauses, it dropped one of the changes...

cdietrich commented 2 days ago

hmmm the patch is not 100% sufficient. something the events in a single filesystemwatcher onDidDelete also misses the events even before they are batched (just logging client side)

cdietrich commented 1 day ago

also can see this in a naive test attempt https://github.com/cdietrich/fs-watcher-bug/blob/main/src/test/extension.test.ts

dbaeumer commented 15 hours ago

@cdietrich good catch. That code should definitely be fixed. The bug got introduced when making notification sending async ;-).

I looked at your test case and that definitely looks like a problem in VS Code itself if you don't receive the 1000 delete events since you are not deleting the parent folder.

dbaeumer commented 15 hours ago

I fixed the problem on the LSP client side. For the VS Code problem I would recommend to open an issue against VS Code. Could you please mention me on it so that I know about the outcome.

EhabY commented 11 hours ago

also can see this in a naive test attempt https://github.com/cdietrich/fs-watcher-bug/blob/main/src/test/extension.test.ts

I rewrote the test using WorkspaceEdits, these also fail on my Windows:

import * as assert from 'assert';

// You can import and use all API from the 'vscode' module
// as well as import your extension to test it
import * as vscode from 'vscode';

suite('Extension Test Suite', () => {
    vscode.window.showInformationMessage('Start all tests.');

    test('Sample test', async () => {
        const uri = vscode.workspace.workspaceFolders![0].uri;
        const wse = new vscode.WorkspaceEdit();
        for (let i = 0; i < 1000; i++) {
            wse.createFile(vscode.Uri.joinPath(uri, `test_${i}.txt`));
        }
        await vscode.workspace.applyEdit(wse);

        const relativePattern = new vscode.RelativePattern(uri, "*.txt");
        const files = await vscode.workspace.findFiles(relativePattern);
        assert.strictEqual(files.length, 1000);

        const watcher = vscode.workspace.createFileSystemWatcher(relativePattern);

        let counter = 0;
        watcher.onDidDelete((e) => {
            console.log(e);
            counter++;
        });

        const deleteWse = new vscode.WorkspaceEdit();
        for (let i = 0; i < 1000; i++) {
            deleteWse.deleteFile(vscode.Uri.joinPath(uri, `test_${i}.txt`));
        }   
        await vscode.workspace.applyEdit(deleteWse);

        const files2 = await vscode.workspace.findFiles("*.txt");
        assert.strictEqual(files2.length, 0);
        watcher.dispose();

        assert.strictEqual(1000, counter);
    });
});