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

[BUGFIX] Destroy proxy object in BelongsTo relation when no longer needed. #5554

Closed
wants to merge 2 commits into from
Closed

[BUGFIX] Destroy proxy object in BelongsTo relation when no longer needed. #5554

wants to merge 2 commits into from

Conversation

pieter-v
Copy link
Contributor

@pieter-v pieter-v commented Aug 3, 2018

The _promiseProxy object that is used by the belongsTo relationship is not destroyed when no longer needed.

This _promiseProxy keeps a reference to the internalModel. This especially a problem if the record is deleted while the other side of the relationship is still alive.

This results for us in a large memory leak: our application has a small set of long living records and during the lifetime it receives a large number of records (those records will be deleted after a while) that has a belongsTo reference to the records in the first set.

There is also a memory leak when proxies are destroyed when the proxied object is still alive. (reported as a separate issue in ember.js)

@runspired
Copy link
Contributor

Thanks! The test failure here is an interesting one, will investigate it.

…verage in other PRs to verify that a proxy-update fix is not also required)
@runspired
Copy link
Contributor

Investigated and pushed a fix, but I'm skeptical that the fix is safe to do without landing some of the other fixes and tests that are pending for async belongsTo relationships. I'm going to hold off on this one until those land.

@runspired
Copy link
Contributor

runspired commented Aug 8, 2018

This PR will depend on

#5534 is especially important, and coverage within it should be expanded to ensure that we test various scenarios in which the belongs-to state changes.

@pieter-v
Copy link
Contributor Author

@runspired Any chance, this gets merged?

@runspired
Copy link
Contributor

@pieter-v yes! would love to clean this up and see if it still helps us. Seems likely that we've already fixed this with some other recent work, but would be good to double confirm!

@runspired runspired self-assigned this Jul 10, 2019
@pieter-v
Copy link
Contributor Author

@runspired Sorry for the delay
Indeed, the proxy leak seems to be fixed. Thanks
But there stills seems to be another leak.
See #6271 for more info.

@runspired
Copy link
Contributor

Closing this as it was resolved by other PRs.

@runspired runspired closed this Aug 24, 2019
@pieter-v pieter-v deleted the relation-leak branch May 18, 2020 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants