-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
vscode: fix local devel and remove disposables memory leak on server restrart #3725
Conversation
df2493d
to
e0e459a
Compare
} | ||
} | ||
await activate(context); | ||
}; |
There was a problem hiding this comment.
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 = []
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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);
}
}
editors/code/src/util.ts
Outdated
|
||
if (disposable instanceof Array) { | ||
while (disposable.length > 0) { | ||
dispose(disposable.pop()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disposing should always be wrapped in try/catch, so that panic in one dtor does not prevent other from firing.
See a fun bug in std about this issue: rust-lang/rust#67235
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a recursive call where try/catch is inside of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh.... yeah, this is exactly my point about control flow being non-obvious :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want inline it here and in Map case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand why we need anything beyond the original loop in the command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just less boilerplate code... If you don't like it I'll remove
The value of releaseTag is not undefined, but null in actual package.json
The memory leak was because on the server restrart the array of extensionContext.substiptions was not cleared
e0e459a
to
261ef1c
Compare
bors r+ |
Build succeeded |
No description provided.