Skip to content

vscode: fix local devel and remove disposables memory leak on server restrart #3725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 26, 2020
Merged

vscode: fix local devel and remove disposables memory leak on server restrart #3725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68ff71e
vscode: fix local devel
Veetaha Mar 25, 2020
e1a5e95
vscode: fix memory leak on server restart
Veetaha Mar 26, 2020
261ef1c
vscode: small post-refactor
Veetaha Mar 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion editors/code/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export class Config {

readonly package: {
version: string;
releaseTag: string | undefined;
releaseTag: string | null;
enableProposedApi: boolean | undefined;
} = vscode.extensions.getExtension(this.extensionId)!.packageJSON;

Expand Down
42 changes: 20 additions & 22 deletions editors/code/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,19 @@ export async function activate(context: vscode.ExtensionContext) {
ctx = await Ctx.create(config, context, serverPath);

// Commands which invokes manually via command palette, shortcut, etc.
ctx.registerCommand('reload', (ctx) => {
return async () => {
vscode.window.showInformationMessage('Reloading rust-analyzer...');
// @DanTup maneuver
// https://github.com./microsoft/vscode/issues/45774#issuecomment-373423895
await deactivate();
for (const sub of ctx.subscriptions) {
try {
sub.dispose();
} catch (e) {
log.error(e);
}

// Reloading is inspired by @DanTup maneuver: https://github.com./microsoft/vscode/issues/45774#issuecomment-373423895
ctx.registerCommand('reload', _ => async () => {
void vscode.window.showInformationMessage('Reloading rust-analyzer...');
await deactivate();
while (context.subscriptions.length > 0) {
try {
context.subscriptions.pop()!.dispose();
} catch (err) {
log.error("Dispose error:", err);
}
await activate(context);
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep this loop here, than wrap it into an abstraciton. This is another case where we do a horrible side-effectful thing, so not having to goto def to figure the control flow helps quite a bit. From the issue comment, seems like we need just to do ctx.subscriptions = []?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subscriptions is marked as read-only, so it cannot be reassigned

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

            while (ctx.subscriptions.length > 0) {
                const sub = ctx.subscriptions.pop()!
                try {
                    sub.dispose();
                } catch (e) {
                    log.error(e);
                }
            }

}
await activate(context).catch(log.error);
});

ctx.registerCommand('analyzerStatus', commands.analyzerStatus);
Expand Down Expand Up @@ -96,7 +94,7 @@ export async function activate(context: vscode.ExtensionContext) {
}

export async function deactivate() {
await ctx?.client?.stop();
await ctx?.client.stop();
ctx = undefined;
}

Expand All @@ -110,11 +108,13 @@ async function bootstrap(config: Config, state: PersistentState): Promise<string
}

async function bootstrapExtension(config: Config, state: PersistentState): Promise<void> {
if (config.package.releaseTag === undefined) return;
if (config.package.releaseTag === null) return;
if (config.channel === "stable") {
if (config.package.releaseTag === NIGHTLY_TAG) {
vscode.window.showWarningMessage(`You are running a nightly version of rust-analyzer extension.
To switch to stable, uninstall the extension and re-install it from the marketplace`);
void vscode.window.showWarningMessage(
`You are running a nightly version of rust-analyzer extension. ` +
`To switch to stable, uninstall the extension and re-install it from the marketplace`
);
}
return;
};
Expand Down Expand Up @@ -169,9 +169,7 @@ async function bootstrapServer(config: Config, state: PersistentState): Promise<
log.debug("Checked binary availability via --version", res);
log.debug(res, "--version output:", res.output);
if (res.status !== 0) {
throw new Error(
`Failed to execute ${path} --version`
);
throw new Error(`Failed to execute ${path} --version`);
}

return path;
Expand All @@ -185,7 +183,7 @@ async function getServer(config: Config, state: PersistentState): Promise<string
}
return explicitPath;
};
if (config.package.releaseTag === undefined) return "rust-analyzer";
if (config.package.releaseTag === null) return "rust-analyzer";

let binaryName: string | undefined = undefined;
if (process.arch === "x64" || process.arch === "ia32") {
Expand Down