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

Fix memory leak in MapLoadEvent #13116

Conversation

kamil-sienkiewicz-asi
Copy link
Contributor

Hey @stepankuzmin, as promised there is my PR fixing memory leak in mapbox. Right now whole Map is being cleaned when it's unmounted.

That's actually pretty funny how this line

postMapLoadEvent(this._getMapId(), this._requestManager._skuToken, this._requestManager._customAccessToken, () => {});

caused memory leak, function () => {} passed as error callback has access to this which is actually a Map object. Function was never cleaned therefore it was holding everything alive :D To my understanding any definition of function like so:

function errCb(){}

postMapLoadEvent(this._getMapId(), this._requestManager._skuToken, this._requestManager._customAccessToken, errCb);

would also leak as errCb also has indirect access to whatever is in _authenticate function which ultimately holds this.painter.context.gl but that's something I'm not sure about.

Hope it makes sense 💯

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Looks good to me! Alternatively we could also modify the postMapLoadEvent code so that it doesn't accept a callback since it's completely unused, but not sure about this — @stepankuzmin do we want to possibly handle errors properly here in the future?

@stepankuzmin
Copy link
Contributor

Excellent, thanks for shipping this, @kamil-sienkiewicz-asi!

@mourner I think we should be safe to delete unused callback, as we don't have plans to handle those errors. However, we should discuss it before removing it, so I think we could merge this and get back to it later 👍

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.

3 participants