Domains async_hook based implementation leaks active domain on resources maintained by internal pools

[atondelier]

• 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

[addaleax]

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]

You're right @addaleax. I had not checked whether the HTTPParser was reused in Node v12.0.0 or not, and yes, by creating a new AsyncResource each 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 AsyncResource would have theorically no impact on existing code in the NodeJS codebase?

[jasnell]

Ping @addaleax @atondelier ... is this issue resolved?

[addaleax]

@Qard @puzpuzpuz @Flarna We’re not doing resource anymore, are we? So this can be closed?

[Qard]

Yep. This should be solved by ensuring reuse always has a unique resource object.