fix: memory leak event error is not unsubscribing #8591
Conversation
the event listener to scene `renderError` is only subscribing. causing memory leak upon cesium `.destory()`
|
Thanks for the pull request @jony89!
Reviewers, don't forget to make sure that:
|
|
@emackey ? |
|
@jony89 Sorry no one has reviewed or replied to this so far. I'll take a look now and thank you for the PR. |
| @@ -671,6 +672,9 @@ import getElement from '../getElement.js'; | |||
| * removing the widget from layout. | |||
| */ | |||
| CesiumWidget.prototype.destroy = function() { | |||
| if (this._scene) { | |||
| this._scene.renderError.removeEventListener(this._onRenderError); | |||
There was a problem hiding this comment.
Since we are destroying the scene on the next line, why do we need to unsubscribe the event listener? Destroying the scene should ensure that renderError and any subscriptions get cleaned up. Do you have an example that shows that's not happening?
There was a problem hiding this comment.
Take any scene destroy it, and do heap delta between the stages (could be done easily with chrome devtool) and you will see that all the references to the scene are still exist in the memory. because of this event. it never unsubscribes and the references are kept. even if scene is being destroyed, someone is still holding the reference to this callback, so it will never be cleaned by the GC.
There was a problem hiding this comment.
Sorry for the slow roll here, I somehow missed this response. While I'd still like to understand the circular dependency causing the GC not to clean up the memory, no reason we can't err on the side of caution here and properly clean up after ourselves. I merged in master and updated CHANGES. I'll merge as soon as CI is happy. Thanks @jony89
There was a problem hiding this comment.
It's less of a circular dependency more like closures and references that stay alive while the anonymous function still have reference. I've discovered it by using heap snapshots and their deltas, one snapshot before destroying the viewer and one afterwards.
I have also tested that code and indeed it removes these references
Thanks @mramato .
|
Thanks again for your contribution @jony89! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
|
Thanks again for your contribution @jony89! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
# Conflicts: # Source/Widgets/CesiumWidget/CesiumWidget.js
the event listener to scene
renderErroris only subscribing. causing memory leak upon cesium.destory()