Use of WeakRef for resource

[betalb]

Is it possible to use WeakRef for storing reference to resource?

I've observed one issue with undici library that relies on FinalizationRegistry to call destroy of async resource, but why-is-node-running is keeping strong reference to resource, preventing it from being garbage collected and destroyed.

[jonkoops]

The documentation states the following about storing a resource in a WeakMap:

In some cases the resource object is reused for performance reasons, it is thus not safe to use it as a key in a WeakMap or add properties to it.

I am not quite sure if this would prevent us from using a WeakRef here, I would still assume that the objects would be destroyed fully and not re-used in this scenario. Only way to find out is to try.

If you have a reproducible case could you share it here? Preferably a small code example that does not rely on external dependencies that we could use for testing.

[jonkoops]

Looks like MDN also explicitly states that a WeakRef might not achieve desired results in regards to being collected on time.

[jonkoops]

This seems promising though:

If the target of a WeakRef is also in a FinalizationRegistry, the WeakRef's target is cleared at the same time or before any cleanup callback associated with the registry is called; if your cleanup callback calls deref on a WeakRef for the object, it will receive undefined.

Let's give this a try.

[jonkoops]

@betalb would you be able to test the change under #85 and let me know if it helps?

[betalb]

@jonkoops I've tested the change and it haven't solved an issue. But then I've realized, that when I was experimenting with this change myself, I also removed stack-trace capturing code and it turned out to be the culprit as well.

So hard reference was stored in map directly in resource field, as well as in stack trace frames. Can't be sure that every stack trace will expose such problems, but in attached sample app it is.

I've created commit that changes stack trace capturing code and stores only line number + filename: betalb@5b6c964

And the sample-app.tar.gz, it can be launched with node --expose-gc index.js. At the end it should print Total undici requests: 0 and number of requests should be 0, though GC is tricky and potentially it may be more, but any value less than 5 is good. With original version it will print Total undici requests: 5, which means that none of undici async resources were GC'ed.

Version of node that I was using v23.4.0 on ARM based mac.

[jonkoops]

How very strange, I would not expect this API to be this leaky, that almost sound like a bug in Node.js itself to me. To be fair, the async_hooks API we rely on is considered deprecated and we should likely move away from it (if possible, see #64).

I'd be amicable to include your proposed fix in the meantime, as we don't really need to hold on to the actual resource itself. Would you be willing to submit a PR so it can be attributed to you @betalb?

[betalb]

Sure, I can do this, but after 6th of Jan

[jonkoops]

Sure, no rush. Let me know if you cannot and I will take care of it 👍