wasi-http: Make child fields immutable

[elliottt]

This PR adds a new case to the header-error variant, immutable, which indicates that an operation that would modify the fields value has failed because that fields is immutable. All fields returned by getter methods on any of the request/response types are considered immutable, while fields that have been created directly through the use of the fields constructor are considered mutable until ownership is passed to an outgoing request or response.

This change paves the way to runtime validation of the Content-Length header, ensuring that it's safe to check the value provided when an outgoing request or response is created.

[pchickey]

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

[elliottt]

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.

[tschneidereit]

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

[elliottt]

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.

[elliottt]

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.

[tschneidereit]

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.

[elliottt]

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?

[tschneidereit]

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

[pchickey]

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?

[tschneidereit]

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.

[elliottt]

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.

[tschneidereit]

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?