Skip to content
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

Memory Leak when using MessageService#showProgress on Backend #13253 #13254

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

jfaltermeier
Copy link
Contributor

@jfaltermeier jfaltermeier commented Jan 10, 2024

What it does

  • dispose cancellation event listeners in RpcProtocol

Fixes #13253

How to test

See reproducer in #13253

Follow-ups

/

Review checklist

Reminder for reviewers

@jfaltermeier jfaltermeier force-pushed the issues/13253_leak-progress branch from bb95927 to 5fd6b5e Compare January 10, 2024 12:40
* dispose cancellation event listeners in RpcProtocol
@jfaltermeier jfaltermeier force-pushed the issues/13253_leak-progress branch from 5fd6b5e to 72119db Compare January 10, 2024 12:54
@jfaltermeier jfaltermeier marked this pull request as ready for review January 10, 2024 13:22
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Change looks good to me and works great, thank you Johannes! I just have a minor issue with the naming but then we can merge this.

/**
* Wrapper for a {@link Disposable} that is not available immediately.
*/
export class MaybeDisposable {
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like the idea of this class and I believe we can even add it to the common disposable.ts. From my understanding it is a wrapper that you mark as disposed and either the already given disposable or any later set disposable is then also disposed, making sure that whatever is given to the wrapper has the correct disposed state.

So I think that this class should actually also implement Disposable so it actually acts more like a wrapper fulfilling the same interface. Also, from the name alone it is not very clear what this class does, so maybe something like DelayedDisposable or actually WrappedDisposable or DisposableWrapper might be better. I believe we could even see this as a special case of the DisposableCollection with an adapted push that does not add things to the collection once dispose was called but instead simply disposes anything that is added to it, so maybe we could also adapt it to that. That would also solve the problem of people calling setDisposable multiple times without knowing whether we already wrapped a disposable or not.

However, I'll leave it up to you, I just think we should share it in a more common API and fix the naming a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll move it to disposable.ts. I am not sure about DisposableCollection though. If something like this is needed with a collection I would rather expect people placing the wrapper inside a disposable collection, which is possible now with implementing Disposable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'm fine with that.

* renamed MaybeDisposable to DisposableWrapper
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

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

Changes look good to me now, thank you very much Johannes!

@jfaltermeier jfaltermeier merged commit e0d4983 into master Jan 15, 2024
14 checks passed
@jfaltermeier jfaltermeier deleted the issues/13253_leak-progress branch January 15, 2024 14:29
@github-actions github-actions bot added this to the 1.46.0 milestone Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Memory Leak when using MessageService#showProgress on Backend
2 participants