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

inconsistent future resource ownership #144

Open
peter-jerry-ye opened this issue Jan 23, 2025 · 5 comments
Open

inconsistent future resource ownership #144

peter-jerry-ye opened this issue Jan 23, 2025 · 5 comments

Comments

@peter-jerry-ye
Copy link

Hi! I noticed that the future-trailers has some extra introduction than future-incoming-response:

... and the resource must be dropped before the parent future-trailers is dropped.

This seems a bit impractical from the client side, as this means that we need somehow keep track of the future-trailers even when it is ready and the internal resource is retrieved. Also I wonder if this is a mistake because I saw this PR as it mentions:

This gives implementations the flexibility to transfer ownership of the resulting trailers out of the future-trailers resource.

I'd like to confirm whether this statement is still true on current implementations?

@pchickey
Copy link
Contributor

Thanks for filing this issue. The spec text you quoted does not match the implementation in wasmtime - the fields returned by future-trailers.get is not a child, it has a lifetime independent future-trailers. We can revise the spec text to remove that restriction, but I want to double check with @guybedford that this is not an issue in the jco implementation before we do.

@guybedford
Copy link
Contributor

guybedford commented Jan 24, 2025

In Jco, we don't currently formally define parent relations and instead provide lifetime errors on a case-by-case host implementation basis. So this wouldn't violate any Jco models, that said we haven't yet added trailers support even though in theory the Node.js implementation could support that. I've posted bytecodealliance/jco#549.

@lukewagner
Copy link
Member

Until WIT interfaces can explicitly capture the parent/child relationship between handles in a way that can inform automatic bindgen (which I'd like to work on after the dust settles with async), I can see how these trapping parent/child relationships can cause headaches. I think I remember @calvinrp saying he had been actively running into this problem and it was a pain point.

If this is a problem that we want to solve before we have explicit parent/child relationships in WIT, one idea is that instead of trapping when a child outlives the parent, we could instead say that, when the parent handle is dropped, any living child handles get put into a "detached" state in which all operations on the child handle immediately return failure (no trap). In this detached state, the child handle is still allocated in the handle table (so there is no use-after-free hazards) until it is explicitly dropped.

@tschneidereit
Copy link
Member

instead of trapping when a child outlives the parent, we could instead say that, when the parent handle is dropped, any living child handles get put into a "detached" state in which all operations on the child handle immediately return failure (no trap).

Wouldn't that require changing any methods on a handle that could be affected to return a result instead of whatever they're currently returning? E.g. wouldn't calling has(name) on the fields handle returned by incoming-request now have to return result<bool, some-error>?

And more generally, it seems like this wouldn't be a backwards-compatible change: existing content that would currently trap (perhaps in some exceptional, cold path) would instead continue running, but later have unhandled errors occurring.

@lukewagner
Copy link
Member

Wouldn't that require changing any methods on a handle that could be affected to return a result instead of whatever they're currently returning?

Maybe in the limit, but we could also (as the authors of the domain-specific interface) map the "detached" state to values of the existing return type (say, saying that has(name) on a detached handle returns false).

And more generally, it seems like this wouldn't be a backwards-compatible change: [...]

Our usual definition of "backwards compatibility" in wasm allows for changing cases that trap to not trapping, and so I think this would be no different.

To be clear, I don't love this option, it's just an option to consider if the current state leads to angry users.

But there's also a milder alternative that is a sortof midpoint between the current state and what I just said: we could say (in wasi-http) that dropping/transferring a parent handle while it has extant children puts the child handles into a "detached" state (instead of trapping), but any use of a detached child handle (for a method call) immediately traps. That option is simpler (it's mechanical, so probably could go into bindgen) and feels vaguely "safer" so it worth trying first to see if it's "good enough" if indeed we have a problem with the current state.

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

No branches or pull requests

5 participants