For #3706 ? No, I think the callbacks need to be stored in the instance itself, as properties. We could have a "__callbacks" property that points to an Array, but that would be externally accessible (bad). The solution to having these kind of "private" properties is to use Symbols as key, instead of a string.

For #3706 ? No, I think the callbacks need to be stored in the instance itself, as properties. We could have a "__callbacks" property that points to an Array, but that would be externally accessible (bad). The solution to having these kind of "private" properties is to use Symbols as key, instead of a string.

Yes I mean this issue. I have done an commit. Do mean more something more like this?

Yes, that looks like a good direction. The main issue I have is that this only works for the wrapper. If we make the interpreter public, then users of that will still have memory leaks.

Does it pass the test? If yes, can you add this also to the CI?

Yes, that looks like a good direction. The main issue I have is that this only works for the wrapper. If we make the interpreter public, then users of that will still have memory leaks.

Does it pass the test? If yes, can you add this also to the CI?

Yes that's true it won't fix it for the ComponentInstance it self. What could work is do write an wrapper for the ComponentInstance that implements the corresponding behavior. So it would work for both. What do you think?

Yes it works with the tests.

I don't think this will work.
The problem is not in the .ts, but it is in the .rs.

the set_callback function will move a RefCountedReference to the binding, so we have this cyclic reference:
RefCountedReference -> JS function -> instance -> native component -> binding -> RefCountedReference -> ...

We need to break the cycle somehow.

I'm not familiar about napi, but could this help us? https://docs.rs/napi/latest/napi/bindgen_prelude/struct.WeakReference.html
otherwise, we should have the object as a property exposed to JS and use that. So it would look like

native component -> array -> user js function -> instance -> native component.

And that cycle is ok because everything is managed by the garbage collector, there is no root there. (while the RefCountedReference introduces a root)

I don't think this will work. The problem is not in the .ts, but it is in the .rs.

the set_callback function will move a RefCountedReference to the binding, so we have this cyclic reference: RefCountedReference -> JS function -> instance -> native component -> binding -> RefCountedReference -> ...

Florian's patch does indeed create a cyclic reference, because it keeps a strong reference to the (new) closure that however still captures the outer environments that hold the instance.

I'm convinced that the Symbol approach is the right way to create a "private" property. I do agree that this needs to be solved in .rs. What I'd do is (in Rust code, using napi API):

(1) Create a JS array that has N entries where N is the number of callbacks (it's a fixed number). Every callback shall have a corresponding index in the array.
(2) Create a Symbol.
(3) Store the array as property in the instance JS object with the symbol as key.
(4) In set callback, determine the index for the callback (given by name). Store the provided JS closure at the given index in the JS array. Capture the index in the Rust closure installed.
(5) When the Rust closure gets invoked, fetch the array, fetch the JS closure at the captured index, invoke it.

That way the JS GC has a full view over the cyclic reference and can collect appropriately.