Skip to content

Commit

Permalink
Eagerly compute SHA's for watched files (#2861)
Browse files Browse the repository at this point in the history 
### Motivation

This PR fixes two issues related to automated restarts. The first one is preventing undesired restarts immediately after the server finishes booting.

The second one is preventing unwanted restarts if a create event is fired for an existing file. In theory, this shouldn't happen, but our tests tell a different story and we consistently see a create event getting fired for updates.

### Implementation

The two fixes are:

1. Unified all of our watchers to only check the top level configuration files and then started eagerly computing the SHAs for all of them (if they are present). This will prevent unwanted restarts if these files are touched immediately after boot
2. Started using the hash check for `didCreate` as well. If the file being updated fires a create event and the content SHA matches

### Automated Tests

Added a new test and reduced some of the sleeps on the other test, which is taking a bit too long.
  • Loading branch information
@vinistock
vinistock authored Nov 19, 2024
1 parent 6bdc52d commit 7d067e5
Show file tree
Hide file tree
Showing 2 changed files with 96 additions and 43 deletions.
44 changes: 40 additions & 4 deletions vscode/src/test/suite/workspace.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,15 @@ suite("Workspace", () => {

await workspace.activate();

for (let i = 0; i < 5; i++) {
await new Promise((resolve) => setTimeout(resolve, 200));
for (let i = 0; i < 4; i++) {
await new Promise((resolve) => setTimeout(resolve, 50));
fs.writeFileSync(path.join(gitDir, "rebase-apply"), "1");
await new Promise((resolve) => setTimeout(resolve, 200));
await new Promise((resolve) => setTimeout(resolve, 50));
fs.rmSync(path.join(gitDir, "rebase-apply"));
}

// Give enough time for all watchers to fire and all debounces to run off
await new Promise((resolve) => setTimeout(resolve, 10000));
await new Promise((resolve) => setTimeout(resolve, 6000));

startStub.restore();
restartSpy.restore();
Expand All @@ -77,4 +77,40 @@ suite("Workspace", () => {
// The restart call only happens once because of the debounce
assert.strictEqual(restartSpy.callCount, 1);
}).timeout(60000);

test("does not restart when watched files are touched without modifying contents", async () => {
const lockfileUri = vscode.Uri.file(
path.join(workspacePath, "Gemfile.lock"),
);
const contents = Buffer.from("hello");
await vscode.workspace.fs.writeFile(lockfileUri, contents);

const workspace = new Workspace(
context,
workspaceFolder,
FAKE_TELEMETRY,
() => {},
new Map(),
);

await workspace.activate();

const startStub = sinon.stub(workspace, "start");
const restartSpy = sinon.spy(workspace, "restart");

for (let i = 0; i < 4; i++) {
const updateDate = new Date();
fs.utimesSync(lockfileUri.fsPath, updateDate, updateDate);
await new Promise((resolve) => setTimeout(resolve, 50));
}

// Give enough time for all watchers to fire and all debounces to run off
await new Promise((resolve) => setTimeout(resolve, 6000));

startStub.restore();
restartSpy.restore();

assert.strictEqual(startStub.callCount, 0);
assert.strictEqual(restartSpy.callCount, 0);
}).timeout(10000);
});
95 changes: 56 additions & 39 deletions vscode/src/workspace.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@ import {
} from "./common";
import { WorkspaceChannel } from "./workspaceChannel";

const WATCHED_FILES = [
"Gemfile.lock",
"gems.locked",
".rubocop.yml",
".rubocop",
];

export class Workspace implements WorkspaceInterface {
public lspClient?: Client;
public readonly ruby: Ruby;
Expand Down Expand Up @@ -58,8 +65,6 @@ export class Workspace implements WorkspaceInterface {
this.createTestItems = createTestItems;
this.isMainWorkspace = isMainWorkspace;
this.virtualDocuments = virtualDocuments;

this.registerRestarts(context);
}

// Activate this workspace. This method is intended to be invoked only once, unlikely `start` which may be invoked
Expand All @@ -82,6 +87,20 @@ export class Workspace implements WorkspaceInterface {
rootGitUri,
".git/{rebase-merge,rebase-apply,BISECT_START,CHERRY_PICK_HEAD}",
);

this.registerRestarts();

// Eagerly calculate SHAs for the watched files to avoid unnecessary restarts
for (const file of WATCHED_FILES) {
const uri = vscode.Uri.joinPath(this.workspaceFolder.uri, file);
const currentSha = await this.fileContentsSha(uri);

if (!currentSha) {
continue;
}

this.restartDocumentShas.set(uri.fsPath, currentSha);
}
}

async start(debugMode?: boolean) {
Expand Down Expand Up @@ -164,7 +183,9 @@ export class Workspace implements WorkspaceInterface {
// server can now handle shutdown requests
if (this.needsRestart) {
this.needsRestart = false;
await this.restart();
await this.debouncedRestart(
"a restart was requested while the server was still booting",
);
}
} catch (error: any) {
this.error = true;
Expand Down Expand Up @@ -342,67 +363,49 @@ export class Workspace implements WorkspaceInterface {
return result;
}

private registerRestarts(context: vscode.ExtensionContext) {
this.createRestartWatcher(context, "Gemfile.lock");
this.createRestartWatcher(context, "gems.locked");
this.createRestartWatcher(context, "**/.rubocop.yml");
this.createRestartWatcher(context, ".rubocop");
private registerRestarts() {
this.createRestartWatcher(`{${WATCHED_FILES.join(",")}}`);

// If a configuration that affects the Ruby LSP has changed, update the client options using the latest
// configuration and restart the server
context.subscriptions.push(
this.context.subscriptions.push(
vscode.workspace.onDidChangeConfiguration(async (event) => {
if (event.affectsConfiguration("rubyLsp")) {
// Re-activate Ruby if the version manager changed
if (
event.affectsConfiguration("rubyLsp.rubyVersionManager") ||
event.affectsConfiguration("rubyLsp.bundleGemfile") ||
event.affectsConfiguration("rubyLsp.customRubyCommand")
) {
await this.ruby.activateRuby();
}

this.outputChannel.info(
"Restarting the Ruby LSP because configuration changed",
);
await this.restart();
await this.debouncedRestart("configuration changed");
}
}),
);
}

private createRestartWatcher(
context: vscode.ExtensionContext,
pattern: string,
) {
private createRestartWatcher(pattern: string) {
const watcher = vscode.workspace.createFileSystemWatcher(
new vscode.RelativePattern(this.workspaceFolder, pattern),
);

// Handler for only triggering restart if the contents of the file have been modified. If the file was just touched,
// but the contents are the same, we don't want to restart
const debouncedRestartWithHashCheck = async (uri: vscode.Uri) => {
const fileContents = await vscode.workspace.fs.readFile(uri);
const fsPath = uri.fsPath;
const currentSha = await this.fileContentsSha(uri);

const hash = createHash("sha256");
hash.update(fileContents.toString());
const currentSha = hash.digest("hex");

if (this.restartDocumentShas.get(fsPath) !== currentSha) {
if (currentSha && this.restartDocumentShas.get(fsPath) !== currentSha) {
this.restartDocumentShas.set(fsPath, currentSha);
await this.debouncedRestart(`${pattern} changed`);
await this.debouncedRestart(`${fsPath} changed, matching ${pattern}`);
}
};

const debouncedRestart = async () => {
await this.debouncedRestart(`${pattern} changed`);
const debouncedRestart = async (uri: vscode.Uri) => {
this.restartDocumentShas.delete(uri.fsPath);
await this.debouncedRestart(`${uri.fsPath} changed, matching ${pattern}`);
};

context.subscriptions.push(
this.context.subscriptions.push(
watcher,
watcher.onDidChange(debouncedRestartWithHashCheck),
watcher.onDidCreate(debouncedRestart),
// Interestingly, we are seeing create events being fired even when the file already exists. If a create event is
// fired for an update to an existing file, then we need to check if the contents still match to prevent unwanted
// restarts
watcher.onDidCreate(debouncedRestartWithHashCheck),
watcher.onDidDelete(debouncedRestart),
);
}
Expand All @@ -416,9 +419,9 @@ export class Workspace implements WorkspaceInterface {
this.#inhibitRestart = true;
};

const stop = async () => {
const stop = async (uri: vscode.Uri) => {
this.#inhibitRestart = false;
await this.debouncedRestart(`${glob} changed`);
await this.debouncedRestart(`${uri.fsPath} changed, matching ${glob}`);
};

this.context.subscriptions.push(
Expand All @@ -429,4 +432,18 @@ export class Workspace implements WorkspaceInterface {
workspaceWatcher.onDidDelete(stop),
);
}

private async fileContentsSha(uri: vscode.Uri): Promise<string | undefined> {
let fileContents;

try {
fileContents = await vscode.workspace.fs.readFile(uri);
} catch (error: any) {
return undefined;
}

const hash = createHash("sha256");
hash.update(fileContents.toString());
return hash.digest("hex");
}
}

0 comments on commit 7d067e5

Please sign in to comment.