Memory leak in _contexts
#11
Comments
|
I looked into this further, and it seems destroy is just never called for some asyncIds. Reading other issues from Node it seems this is caused by the promises not being destroyed until GC. However for some reason they seem to never be gc'ed, and I am not sure how to debug that. |
|
I'm looking into something we can do with the context's |
|
There is a similar looking issue in Node, see nodejs/node#14446 (comment). This looks from a quick glance to be the same issue. |
|
Hi, any news about this issue, what should be done in the code of cls-hooked to solve this one ? |
|
Hum, seems like, it is a normal behavior in this issue nodejs/node#14446 (comment) so maybe this issue here is OK too. |
|
We eventually worked out what was causing this. Turned out to actually be the NewRelic that was the main culprit, albeit we also had some of our own. The fact that the contexts are only destroyed when the promise is garbage collected, requires you to be very careful that you do not in some indirect way reference the promises from the data you store in the cls. |
While it doesn't make any difference now. In the future PromiseHooks could be refactored to provide an asyncId instead of the promise object. That would make escape analysis on promises possible. Escape analysis on promises could lead to a more efficient destroy hook, if provide by PromiseHooks as well. But at the very least would allow the destroy hook to be emitted earlier. The destroy hook not being emitted on promises frequent enough is a known and reported issue. See nodejs#14446 and Jeff-Lewis/cls-hooked#11. While all this is speculation for now, it all depends on the promise object not being a part of the PromiseWrap resource object. Ref: nodejs#14446 Ref: nodejs/diagnostics#188
While it doesn't make any difference now. In the future PromiseHooks could be refactored to provide an asyncId instead of the promise object. That would make escape analysis on promises possible. Escape analysis on promises could lead to a more efficient destroy hook, if provide by PromiseHooks as well. But at the very least would allow the destroy hook to be emitted earlier. The destroy hook not being emitted on promises frequent enough is a known and reported issue. See #14446 and Jeff-Lewis/cls-hooked#11. While all this is speculation for now, it all depends on the promise object not being a part of the PromiseWrap resource object. Ref: #14446 Ref: nodejs/diagnostics#188 PR-URL: #23443 Refs: #14446 Refs: nodejs/diagnostics#188 Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>
We upgraded to Node 8 yesterday, and are now seeing memory leaks. Taking some heap snapshots I can see the
_contextsmap growing all the time. We run 4.2.2, so it shouldn't be the memory leak fixed recently.It's hard to follow the code, but I would guess this is related to how the promises are handled. At least it seems the contexts are not always removed from the map.
The text was updated successfully, but these errors were encountered: