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

Leak fix in tabs-main.ts #14186

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Conversation

dfriederich
Copy link
Contributor

The disconnect from the signal was not passing the this reference as it was used in the connect, hence the disconnect failed to find the handler.

When developing a theia based app we run into leaks due to TabsMainImpl keeping our custom control
in memory even when it was closed. The leak was through the controls title, through TabsMainImpl.
This PR fixes the main issue, which was leaking every control for us.

However the TabsMainImpl still is not optimal as it only releases references when it rebuilds its model,
and that does not happen when closing a tab.

The disconnect from the signal was not passing the this reference
as it was used in the connect, hence the disconnect failed to find
the handler.
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thanks looks good to me 👍

However the TabsMainImpl still is not optimal as it only releases references when it rebuilds its model, and that does not happen when closing a tab.

I plan to refactor the code that handles the tab API and the whole editor manager eventually anyway. This will likely be fixed by that change.

@msujew msujew added the quality issues related to code and application quality label Sep 18, 2024
@msujew msujew merged commit 4a0228b into eclipse-theia:master Sep 18, 2024
11 checks passed
@dfriederich
Copy link
Contributor Author

I plan to refactor the code that handles the tab API and the whole editor manager eventually anyway.
This will likely be fixed by that change.

The leak is quite unfortunate IMHO, wonder how bad it would be to recompute the model for any tab removal?
would at least fix the leak for me. I will create an issue to track that it currently leaks.

@martin-fleck-at
Copy link
Contributor

I'm also interested in this, so thank you @dfriederich for creating #14203.
@msujew Do you have any approximate timeline on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality issues related to code and application quality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants