Stream: git-wasmtime

Topic: wasmtime / issue #7463 Change http `consume` methods to s...

Wasmtime GitHub notifications bot (Nov 02 2023 at 20:30):

pchickey commented on issue #7463:

@tschneidereit or @JakeChampion: will this change be ok with the JS embedding? They effectively mean you need to retrieve the status and headers from an incoming-response and the method, path&query, scheme, authority, and headers, from an incoming-request before starting to consume the body, after which you'll not be able to retrieve those elements.

Wasmtime GitHub notifications bot (Nov 02 2023 at 21:37):

tschneidereit commented on issue #7463:

Hmm, while the answer to that is "yes", I would somewhat prefer not to do this, unless we absolutely have to (e.g. because otherwise an efficient implementation isn't viable.)

The reason is that a request semantically isn't just its body, and "just" needing to access the body shouldn't require extracting all other information one might possibly need, just to be on the safe side. (In fact, I'd somewhat prefer for consume to be called body, while still only working once. Or even just always returning the same resource handle, same as headers returns the same resource when called multiple times (I think?). I'm assuming that there are technical or conceptual reasons why that doesn't work, though.

Wasmtime GitHub notifications bot (Nov 03 2023 at 00:47):

alexcrichton commented on issue #7463:

Ok, sounds reasonable to me :+1:

Wasmtime GitHub notifications bot (Nov 03 2023 at 16:56):

pchickey commented on issue #7463:

In fact, I'd somewhat prefer for consume to be called body, while still only working once.
I am in favor of this change!
Or even just always returning the same resource handle, same as headers returns the same resource when called multiple times (I think?). I'm assuming that there are technical or conceptual reasons why that doesn't work, though.

Headers doesn't return the same resource when called multiple times, instead they return a fresh resource containing a HostFields::Ref which refers to the same fields.

I'll do my best to explain this but @alexcrichton please correct or clarify for me: We can't do the "return the same resource multiple times" trick because these returned resources must become owned by the caller component, and therefore have their own independent lifetime. When the caller becomes the owner it must be possible for it to to drop the resource, or move its ownership to another component, without affecting anything else in its table.

We used to gloss over this distinction when passing table indices around, but now its important to remember that table indicies are just the abstraction to resources which are taken care of entirely by the component runtime, and out in our implementations of wit interfaces, that is totally abstracted away.

Wasmtime GitHub notifications bot (Nov 03 2023 at 17:10):

alexcrichton commented on issue #7463:

That's all correct yeah, and I'd add one more bit of context related to headers. Headers are a bit of a leaky abstraction because each time you acquire a set of headers you're actually acquiring a pointer to where the headers store (that's the HostFields::Ref). This means that if the storage of the headers are dropped that becomes in effect a dangling pointer. The leaky abstraction part is that we prevent dropping the storage of the headers while there's active resources pointing at the headers. Only once all headers resources are gone can the original storage be dropped too.

Theoretically something similar is possible for bodies. The catch is that whenever we access headers we've always got the table around so we can chase the "pointer" which is actually just a table index. For I/O objects their operations are not executed with a table as an argument, so an I/O object can't currently use the same strategy.

The other possible solution to enable calling consume (or body) multiple times would be to do reference counting on the host. In Rust though that'd require Arc<Mutex<_>> which is otherwise unnecessary overhead.

Wasmtime GitHub notifications bot (Nov 03 2023 at 18:25):

tschneidereit commented on issue #7463:

Thank you for (re-)explaining the constraints that apply here, Pat and Alex :heart:

Given all this, I think it's probably better to have consume work only once, and bindings/frameworks that want to expose an accessor for the body that can be used multiple times can just implement that in content by caching the result.

Last updated: Nov 22 2024 at 16:03 UTC