wit-bindgen icon indicating copy to clipboard operation
wit-bindgen copied to clipboard

C#: the generated finalization logic does not take child resources into account

Open SingleAccretion opened this issue 1 year ago • 10 comments

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:

  1. 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 SIGSEGV instead of raising a NullReferenceException).
  2. 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).
  3. Something else?

@dicej, @yowl, @pavelsavara (I would also recommend tagging the .NET interop team)

SingleAccretion avatar Aug 28 '24 10:08 SingleAccretion

cc @jsturtevant

pavelsavara avatar Aug 28 '24 10:08 pavelsavara

It will trap with resource has children full trace https://gist.github.com/pavelsavara/400212c6fe25b6bb7b94e6432446c76f

pavelsavara avatar Aug 28 '24 10:08 pavelsavara

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?

jsturtevant avatar Aug 28 '24 16:08 jsturtevant

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.

yowl avatar Aug 28 '24 16:08 yowl

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

pchickey avatar Aug 28 '24 16:08 pchickey

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.

SingleAccretion avatar Aug 28 '24 16:08 SingleAccretion

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").

dicej avatar Aug 28 '24 17:08 dicej

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

pavelsavara avatar Aug 28 '24 17:08 pavelsavara

It would be great to improve resource has children message to at least say handle numbers of children which are still alive.

pavelsavara avatar Aug 28 '24 17:08 pavelsavara

At the moment I'm confused if outgoing-request handle 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.

dicej avatar Aug 28 '24 17:08 dicej