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.

Sign up for GitHub

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

Jump to bottom

MapiResponse never gets garbage collected in Node #396

Open
markgandy opened this issue Sep 9, 2020 · 6 comments · May be fixed by #496
Open

MapiResponse never gets garbage collected in Node #396

markgandy opened this issue Sep 9, 2020 · 6 comments · May be fixed by #496

Comments

@markgandy
Copy link

markgandy commented Sep 9, 2020 •
edited
Loading

We have a memory leak issue with a Node app that appears to be caused by MapiResponse never getting cleaned up by the garbage collecter. We are creating a client and using to get directions as below using Node v12.16:

const mapboxDirectionsFactory = require('@mapbox/mapbox-sdk/services/directions');

class MapboxAdapter {
	constructor(options) {
		this.client = mapboxDirectionsFactory(options);
	}
	directionsConsideringTraffic({geoLocations}) {
		return this.client.getDirections({
			profile: 'driving-traffic',
			geometries: 'geojson',
			steps: true,
			waypoints: geoLocations.map(coordinates => ({coordinates}))
		}).send();
	}
}

module.exports = {
	MapboxAdapter
};

But we can see that MapiRequest and MapiResponse never get cleaned up. Looking at heap snapshots from prod we can see these just keep growing (example here has grown up to 90MB just for MapiResponse)
image

I have also reproduced running locally in the Chrome debugger. I can see these objects build up and never get removed even when I manually run a GC in Chrome.

It looks like the SDK uses the global namespace so I guess this is why they are never GC'd? Is there something special we should be doing in Node to avoid this issue?
@markgandy
Copy link
Author

Just following up on this. We ended up removing the mapbox SDK and calling the directions REST API directly instead. You can see pretty clearly in the graph, I think it's pretty obvious when we made the change!
Heap would just keep growing until eventually out of memory. So looks to definitely be an issue.

image

@kascakm
Copy link

kascakm commented Aug 20, 2021

I'm having the same issue. Looking through the code it seems that this object here requestsUnderway tracks all requests but would remove them only if the request is aborted.
https://github.com/mapbox/mapbox-sdk-js/blob/main/lib/node/node-layer.js#L15

@tetchel
Copy link

tetchel commented Jan 4, 2022

This seems like a significant issue

@mpothier @cmtoomey

Sorry for the tags but I would like to see a maintainer response on this one before adopting this SDK.

@mikksoone
Copy link

We got hit by this as well. Thanks @kascakm for the pointer to the leak - this helped us verify it was the same problem. We also moved to web api and now everything is fine.

@tonda100
Copy link

tonda100 commented Apr 5, 2023

We also have to move rest api because of this issue.

@mehmetmalli mehmetmalli linked a pull request Nov 8, 2024 that will close this issue
Open
@mehmetmalli
Copy link

I created a PR for this: #496

tested locally, seems to fix the memory build up issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: memory leak caused by storing requestsUnderway indefinitely mehmetmalli/mapbox-sdk-js
6 participants
@mikksoone @markgandy @tonda100 @tetchel @kascakm @mehmetmalli