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

[v0.3] Make (potentially all) headers mutable #113

Open
rylev opened this issue Apr 26, 2024 · 10 comments
Open

[v0.3] Make (potentially all) headers mutable #113

rylev opened this issue Apr 26, 2024 · 10 comments

Comments

@rylev
Copy link
Contributor

rylev commented Apr 26, 2024

From this discussion on the v0.3 draft PR @lukewagner said:

#103 is proposes adding a take-headers for extracting headers that become mutable (once they are disconnected from the request/response). Are you taking that addition into account and still it's insufficient? (Alternatively, it seems like we could just make all fields other than Content-Length mutable; the current rules are just trying to be regular, but Content-Length is the only one that really wants the current rules.)

To which @dicej replied:

If I want to pass the original request to the imported wasi:http/handler instead of creating a brand new request and passing that, the only way I can modify headers is if I get mutable access to the headers of the original request (i.e. not a disconnected version), so yes, I think we'd need the mutable-except-for-content-length approach.

Which points to the ability to making headers mutable so that they can changed without having to rebuild request and response objects which could not only be a bit of developer experience issue but also lead to excessive copying.

Additionally, Content-Length is pointed to as the reason for why headers are immutable in the first place. However, in #105, @lukewagner suggests we want to leave it to guests to set the Content-Length header correctly.

While messing with Content-Length header is an easy way to introduce bugs, I don't understand why we fully trust the guest to set the Content-Length header on request/response construction, but we cannot trust the guest to not mess with the Content-Length header inappropriately down the line?

@lann
Copy link
Contributor

lann commented Apr 26, 2024

Another header to consider is Host which you would typically want to be immutable once client TLS config has been prepared (so that it matches SNI).

@lukewagner
Copy link
Member

The idea is that the guest can arbitrarily modify Content-Length and then, once the guest creates a request or response, the host can enforce that the guest actually provides the declared Content-Length bytes (producing the appropriate error otherwise), so we're not actually "trusting" the guest, we're letting the guest declare their intention and then holding the guest to it. But this whole idea only makes sense if the guest can't mutate Content-Length once they've started streaming a body (which we approximate by saying "once the headers have been used to create a request or response). Thus it's really only Content-Length where we need this special immutability rule; I think the rest of (non-forbidden) headers could be made mutable.

In the case of Host, my impression is that it should be in forbidden headers list. Instead, the existing authority parameter would be used by the host to synthesize a Host header as necessary (or not) by the underlying transport (just like Transfer-Encoding and other hop-by-hop headers).

@lann
Copy link
Contributor

lann commented Apr 29, 2024

the guest can't mutate Content-Length once they've started streaming a body (which we approximate by saying "once the headers have been used to create a request or response).

With the current 0.3.0-draft types would it be sufficient to say that headers are copied at the boundaries of the handle function? Request headers would be copied immediately (from the caller's perspective) after handle is called and response headers would be copied immediately (from the callee's perspective) after handle returns. This would only need to be a copy semantically; in practice an implementation could write the headers to the wire immediately and stash the content-length in per-request state.

@lukewagner
Copy link
Member

Even within a single component, if you can mutate Content-Length for an already-created request/response, it creates strange questions without satisfying answers for how and if this Content-Length is enforced for the existing body (which can already have been fully streamed or be in some intermediate state). These questions get even stranger when you consider multi-component scenarios where component A creates a request with a Content-Length X and an output stream to write the body, and then component A transfers the request to component B, which then mutates Content-Length while A is still writing the body. A lot of these examples don't make sense with mutable Content-Length but have an easy answer once Content-Length is fixed upon request/response construction.

@lann
Copy link
Contributor

lann commented Apr 29, 2024

Maybe the answer here is to set the content-length in the body constructor rather than the request/response and forbid it in headers like host.

@lukewagner
Copy link
Member

Yes, that's also an option, and it's one we talked about a while back, because it definitely feels cleaner. A practical consideration we found though is that there's a lot of HTTP-using code that, one way or another, we want to Just Work in the wasi:http/proxy world and that code wants to read and sometimes write Content-Length. Thus, if we forbid Content-Length in WASI's headers, then all the guest code will end up needing to special-case Content-Length in the glue code. And then on the host side Content-Length will often be sitting there in the host's representation, and so it seemed like maybe just a net overhead for everyone. But happy to discuss this alternative again in 0.3.

@tschneidereit
Copy link
Member

@lukewagner I remember that there was a good reason for this, but can you remind me why it's not an option to make headers immutable at the point where they become fundamentally so—i.e. once a request/response is actually sent and the headers are on the wire?

It seems like up to that point, it's entirely fair game to change Content-Length, and we could still provide all the error messages we'd want to. E.g., if Content-Length is < than the actual number of bytes in the body at the point of sending the request/response, or if Content-Length is > than the number of bytes, and the body has been closed, that operation could fail.

@lukewagner
Copy link
Member

@tschneidereit The cool thing about the current transfer semantics of own handles combined with the "headers are a child resource of the request/response" rule is that, once you hand off ownership of a request/response (via incoming-handler or outgoing-handler), you necessarily don't have access to the headers anymore and thus it's not even possible to mutate them.

It's only before that point in time that being able to mutate headers means we have to consider all these cases (like you're enumerating) and introduce custom failures (that can only happen for Content-Length mind you, and thus Content-Length is still "special" among fields). But I expect you're right, we technically could and this would be a bit more permissive than what I'm suggesting above; we'd just need to be careful to enumerate all the possibilities and state transitions. Making Content-Length immutable-once-used-in-a-request-or-response is just my attempt to balance complexity with expressivity, but happy to consider the alternatives and tradeoffs.

@tschneidereit
Copy link
Member

@lukewagner thank you, that helps. I agree that the way ownership transfer currently implies immutability is very nice—but it seems like your proposal gives up on that as well.

Having the semantics be "ownership transfer of a headers resource implies that the Content-Length header, and only that one, becomes immutable, while everything else stays the same" seems way more subtle. Especially given that the resource in question is actually fields, and this special-casing should presumably only apply if it's used as headers, not trailers.

But completely aside from all these questions, it seems to me like this enforces a specific order of operations on code that isn't inherent to the domain: that in keeping Content-Length and the actual contents of the body in sync, one must first ensure the right value for Content-Length, and then supply the contents. (I agree that that is in some sense the "right" order, but not in a way that I think we should be in the business of enforcing.)

@lukewagner
Copy link
Member

But completely aside from all these questions, it seems to me like this enforces a specific order of operations on code that isn't inherent to the domain

JS simply doesn't let you set Content-Length at all and instead Content-Length is set only when it guaranteed upon construction of the Request/Response (by being a Blob or, in CloudFlare, a FixedLengthStream) -- this is the same moment that I'm suggesting that Content-Length become immutable in headers, so no loss of expressivity. Similarly, if we go with the approach @lann mentioned and make content-length a parameter of the request/response constructor, that's also when it gets locked in. So it doesn't seem like "I want to mutate Content-Length later than request/response construction" is really a new dimension of expressivity that we need here (although I'd be curious to hear if any other languages' HTTP libraries do allow late mutation of Content-Length (while also checking that the body length matches, as RFC 9110 specifies).

Overall, it seems like we should either make Content-Length forbidden and a constructor-parameter/getter of request and response or do this "make Content-Length immutable upon construction" thing. I'd be fine with the former, I just wanted to explain why we didn't go with it earlier when it was first suggested.

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

4 participants