Stream: git-wasmtime

Topic: wasmtime / issue #7524 wasi-http: Make child fields immut...

Wasmtime GitHub notifications bot (Nov 10 2023 at 18:37):

elliottt commented on issue #7524:

Instead of checking the immutability flag in the callee, can we factor it into a get_fields that returns a &FieldMap and use that in all the cases that don't require mutability, and then turn get_fields_mut which returns Result<&mut FieldMap, HeaderError> that will only be used for set/append/delete and itself return Err(HeaderError::Immutable)?

Great call! I've reworked this to have two accessors now, and I like it much more.

Wasmtime GitHub notifications bot (Nov 11 2023 at 14:40):

tschneidereit commented on issue #7524:

@elliottt I didn't catch this earlier, but I think it's somewhat unfortunate to take such drastic measures here. Specifically, I would predict that fields entries sometimes being mutable, sometimes not, while in many languages it's not really possible to express that in the type system—and it's not expressed in whether we have a borrowed or mutable handle—will lead to lots of confusion.

What if instead we made it so that headers are passed in as an owned handle when sending a request/response, ensuring that they can't be mutated at all anymore afterwards? For those use cases where the values still need to be accessible, the handle can be cloned. (I'd imagine that we could make that pretty cheap on the host side, by having clone return a Cow, such that if the clone is only ever read from, we don't incur overhead.

Wasmtime GitHub notifications bot (Nov 11 2023 at 18:02):

elliottt commented on issue #7524:

I think your suggestion is what's implemented here: the outgoing-request and outgoing-response constructors both take ownership of the fields they're given, and it's only the fields resources returned by their headers methods that become immutable. The fields.clone method can be used on those handles for the case where it's necessary to mutate a new copy of those fields, but once a fields is associated with a request or response (incoming or outgoing) it becomes immutable.

Wasmtime GitHub notifications bot (Nov 11 2023 at 18:08):

elliottt commented on issue #7524:

More concretely: this doesn't treat individual headers in a special way, it makes it so that the fields resource returned by the headers method on request and response resources immutable as a whole. So if you tried to use fields.set, fields.delete, or fields.append on the fields resource returned by outgoing-response.headers, you would uniformly see the header-error.immutable error returned.

Wasmtime GitHub notifications bot (Nov 11 2023 at 21:57):

tschneidereit commented on issue #7524:

I think ensuring that headers values can't be changed after they become relevant for error handling makes a lot of sense. What I had in mind was to move that point further back, to the moment where the request/response is actually sent. What I mean is not having headers be associated with requests/responses at all until the latter are sent.

Concretely, that would entail four changes:

1. In the outgoing-handler interface, changing the signature of the handle function to

  handle: func(
    request: outgoing-request,
    headers: headers,
    options: option<request-options>
  ) -> result<future-incoming-response, error-code>;

2. In the response-outparam resource type, changing the signature of the set method to

  set: static func(
    param: response-outparam,
    response: result<outgoing-response, headers, error-code>,
  );

3. In the outgoing-request resource type, removing the headers parameter from the constructor and removing the headers method

4. In the outgoing-response resource type, removing the headers parameter from the constructor and removing the headers method

That should make it so that whenever one can get a handle to a headers object, it's certain to be mutable, so that it's not required to carefully keep track of how that handle was acquired.

All this said, I'm not 100% sure myself that this is the better approach, but it does seem to make things more self-explanatory at least.

Wasmtime GitHub notifications bot (Nov 11 2023 at 23:45):

elliottt commented on issue #7524:

That makes sense to me, thank you for the detailed explanation! I'm open to making this change, would it make sense to also make the incoming handler take a fields argument to keep symmetry between the two?

Wasmtime GitHub notifications bot (Nov 12 2023 at 10:20):

tschneidereit commented on issue #7524:

ah, good point yeah: I think that symmetry would make a lot of sense

Wasmtime GitHub notifications bot (Nov 13 2023 at 16:54):

pchickey commented on issue #7524:

Can't you achieve these with library code that just maintains the headers as an owned resource and just creates the outgoing-request at the point of calling outgoing-handler.handle?

Wasmtime GitHub notifications bot (Nov 13 2023 at 17:14):

tschneidereit commented on issue #7524:

Yes, that's possible, but I think there are a couple of downsides.

One is more of a conceptual one: headers as an owned resource has to completely different behaviors depending on whether it's retrieved from a parent resource or created standalone. I think it'd be nicer to have uniform behavior, but that's not that important.

The other is that that means not being able to use the stream at all until after creating the final outgoing-request and sending it off.

Given that the key property we want to get at is to ensure that the headers don't change between setting them and sending off the parent resource, it seems to make sense to me to create that association just in time, by passing them in when sending.

Wasmtime GitHub notifications bot (Nov 13 2023 at 18:01):

elliottt commented on issue #7524:

Thinking about this over the weekend, I think there's a problem with passing the headers to the handler: that means that the value of the Content-Length header could change after the outgoing-body is created, which would allow for the following situation that the current PR prohibits:

1. Create an outgoing request
2. Create a fields with a Content-Length of 20
3. Create an outgoing-body (this is where we would need to know the value of Content-Length, if it's present)
4. Update the Content-Length header to be 10
5. Send the request with those headers
6. Write 20 bytes to the output stream associated with the outgoing-body

We should be able to report an error in step 6 for writing too much, but as the outgoing-body is derived from the outgoing-request, and the headers are no longer associated directly with the request, we miss the opportunity to fetch the most current value of Content-Length. Associating the headers with the request when it's constructed and considering the headers immutable at that point means that we can trust that the outgoing-body produced by the outgoing-request.body method sees the final value for Content-Length when it's created.

Wasmtime GitHub notifications bot (Nov 13 2023 at 18:11):

tschneidereit commented on issue #7524:

What I was envisioning was a modified sequence along these lines:

1. Create an outgoing request
2. Create a fields with a Content-Length of 20
3. Create an outgoing-body (we don't yet need to know anything about Content-Length here)
4. Update the Content-Length header to be 10
5. Send the request with those headers, and set the current value of Content-Length on the outgoing-body
6. Write 20 bytes to the output stream associated with the outgoing-body
7. get an error

Now what I don't at all know is how hard it'd be to move setting the expected body length from step 3 into step 5. Certainly the request has a reference to it still, so ideally it shouldn't be too hard?

Last updated: Dec 23 2024 at 12:05 UTC