C#: the generated finalization logic does not take child resources into account
Currently, the generated resource types have a finalizer, like this.
This logic is not correct if the resource type may have child resources - this will trap on drop. See also this Discord discussion.
(Side note: it is also not correct due to https://github.com/dotnet/runtime/issues/103522, but that's a different story.)
Options:
- Drop the finalizer. It means the user code will be exposed to leaks and traps on incorrect usage, but at least they will be determinisic (finalizers are not deterministic). I will mention that traps in managed code are to be avoided, since they can't be converted to managed exceptions (it's like sending
SIGSEGVinstead of raising aNullReferenceException). - Design some sort of automatic scheme that would close the child resources. This is complicated by the fact that the parent-child relationship is currently only encoded in documentation (it means this can only be done for 'well-known' resource types).
- Something else?
@dicej, @yowl, @pavelsavara (I would also recommend tagging the .NET interop team)
cc @jsturtevant
It will trap with resource has children
full trace https://gist.github.com/pavelsavara/400212c6fe25b6bb7b94e6432446c76f
Reading through the Discord thread, it sounds like there was general agreement to go with option 1 in the short term and force the user to implement via using statements?
Design some sort of automatic scheme that would close the child resources. This is complicated by the fact that the parent-child relationship is currently only encoded in documentation (it means this can only be done for 'well-known' resource types).
Is it worth bringing up in the WASI design?
It seems that in some cases, where the ownership of a child resource is transferred to another resource, then we do know from the wit (no borrow) that we should dispose of the child.
Our goal with child resources is to eventually have a way to encode that relationship in the wit and component model type system, but that mechanism doesn't exist yet, so we had to settle for documenting it. We understand this isn't ideal but this is the first time its come up in bindings generation, to my knowledge. (In other language bindings, behaving correctly gets punted to the user.)
like there was general agreement to go with option 1 in the short term and force the user to implement via using statements?
There is no other option for the short term. But I don't think it is a tenable production solution.
It seems that in some cases, where the ownership of a child resource is transferred to another resource, then we do know from the wit (no
borrow) that we should dispose of the child.
Yes, and wit-bindgen already handles that correctly and automatically by setting the Handle property to zero (meaning "no handle; nothing to dispose of").
At the moment I'm confused if outgoing-request handle needs to be kept alive until end of the response streaming.
Note that in C# the response Content stream is passed to the user before end of streaming.
Users will typically rely on C# GC to do the stream cleanup (much later), but that stream must keep WIT handles alive.
~~If we need to keep also request handles alive, that would mean we are possibly keeping some request related host buffers/objects alive until then. Maybe not, but I'm struggling to understand the parent/child graph just by reading the WITs.~~
Update: resolved
It would be great to improve resource has children message to at least say handle numbers of children which are still alive.
At the moment I'm confused if
outgoing-requesthandle needs to be kept alive until end of the response streaming.
outgoing-handler/handle consumes the outgoing-request handle, so the guest won't have access to it anyway once the request is sent.
Also, you can dispose of the incoming-response after calling consume on it since you will have taken ownership of the the returned incoming-body handle (i.e. it is no longer a child of the incoming-response once you've called consume). The docs could definitely be clearer about that.