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.

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

Use of WeakRef for resource #84

Open
betalb opened this issue Dec 13, 2024 · 5 comments · May be fixed by #85
Open

Use of WeakRef for resource #84

betalb opened this issue Dec 13, 2024 · 5 comments · May be fixed by #85

Comments

@betalb
Copy link

betalb commented Dec 13, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@jonkoops
Copy link
Collaborator

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 added a commit to jonkoops/why-is-node-running that referenced this issue Dec 15, 2024
Closes mafintosh#84

Signed-off-by: Jon Koops <jonkoops@gmail.com>
@jonkoops jonkoops linked a pull request Dec 15, 2024 that will close this issue
@jonkoops
Copy link
Collaborator

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

@betalb
Copy link
Author

betalb commented Dec 19, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants