Repository navigation
Domains async_hook based implementation leaks active domain on resources maintained by internal pools #27550
Description
- Version: >= v9.3.0 & <12.0.0
- Platform: all
- Subsystem: domain
By coupling domains with async_hooks from version 9.3.0, where it assigns the active domain to the async resource, and not unsassigning it, these domains are retained in memory by, for instance, the pool of HTTPParsers.
While the retained domains are very small objects,
- some CLS systems may use the active domain as keys for
WeakMaps, expecting all references to the active domain to disappear after it definitely exits - some web servers may use this CLS system to store requests contexts
- these requests context may retain huge payloads.
Considering any average size of payload, multiplied by 1000 (the size of the parsers pool), this can create a huge memory leak.
While I think it's the job of the application implementor to not create such a reference from an application WeakMap to the active domain, we may prevent this from causing huge leaks by unassigning the domain on any async resource after it has been used.
Otherwise, I may have missed the true reason why this reference is kept.
Thanks
Activity
addaleax commentedon May 3, 2019
I think #25094 addresses this particular issue?
That being said, I think it would make sense to un-assign the domain from the async_hooks destroy hook.
atondelier commentedon May 3, 2019
You're right @addaleax. I had not checked whether the
HTTPParserwas reused in Node v12.0.0 or not, and yes, by creating a newAsyncResourceeach time, and not using resources from pools, this removes the risk of such a leak.Do you confirm that unassigning the domain on any destroyed
AsyncResourcewould have theorically no impact on existing code in the NodeJS codebase?jasnell commentedon Jun 26, 2020
Ping @addaleax @atondelier ... is this issue resolved?
addaleax commentedon Jun 29, 2020
@Qard @puzpuzpuz @Flarna We’re not doing resource anymore, are we? So this can be closed?
Qard commentedon Jun 29, 2020
Yep. This should be solved by ensuring reuse always has a unique resource object.