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.

Sign up for GitHub

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

Jump to bottom

Methods needed to get owned headers out of requests and responses #102

Open
acfoltzer opened this issue Feb 16, 2024 · 8 comments · May be fixed by #103
Open

Methods needed to get owned headers out of requests and responses #102

acfoltzer opened this issue Feb 16, 2024 · 8 comments · May be fixed by #103

Comments

@acfoltzer
Copy link
Contributor

The main case where this is currently painful is when trying to perform modifications while forwarding along an incoming-request as an outgoing-request, or incoming-response as an outgoing-response. The desired workflow is something like:

headers <- incoming.headers();
headers.delete("header-I-want-to-filter");
outgoing <- outgoing-request.new(headers);

Currently, this fails because headers is a child resource of incoming_request where the underlying headers are immutable, so methods like append and delete are not permitted. Invoking outgoing-request.new with this child resource is fine, with embedders able to clone the headers under the hood. But once it's in the outgoing resource it's once again only accessible via an immutable child resource.

As far as I can tell, the only way to modify headers in this proxying scenario is to explicitly clone the headers:

headers <- incoming.headers();
new_headers <- headers.clone();
new_headers.delete("header-I-want-to-filter");
outgoing <- outgoing-request.new(new_headers);

This isn't the end of the world, but it would be nice to avoid this clone and the implicit clone performed by constructing outgoing-request with a child resource by offering a means to get the headers back from these types as an owned resource. Perhaps we could, similar to Rust's http::Request::into_parts() method, augment the consume method to also return the headers (and other parts like method, authority, path, status code [for responses]) along with the body. Strawman signatures:

resource incoming-request {
  consume: func() -> result<(
    method,
    option<scheme>,
    option<string>, // authority
    option<string>, // path-with-query
    headers,
    incoming-body
  )>;
}

resource incoming-response {
  consume: func() -> result<(status-code, headers, incoming-body)>;
}

This will remain relevant even once 0.3.0 arrives with the unification of the incoming- and outgoing- types. If the headers can still only be accessed immutably from an existing value, proxy applications wishing to perform modifications will still have to get a mutable headers resource via some means, and absent other changes that'll mean an unnecessary clone.
@lukewagner
Copy link
Member

I think this is a very good idea. IIRC, @elliottt independently made the same suggestion too. Given how immediate the use case is and how small of an addition, this seems like a good candidate for inclusion in our next minor (0.2.1) release.

@acfoltzer
Copy link
Contributor Author

@lukewagner great! @elliottt do you already have a branch with this or should I prepare a PR?

@elliottt
Copy link
Collaborator

I don't have a branch, and my suggestion was a little less general: I was proposing we add a take-headers method that would transfer ownership of the headers belonging to a request/response. I like your consume idea more, as it covers all fields instead of just the headers.

What do you think about making it a static method that takes an owning reference to the request/response? That way we could be sure that there's no leftover request/response resource that has weird internal state. I suppose that since consume is already part of the api, we would need a new name, but perhaps we could mirror the rust api you mentioned and call it into-parts?

@acfoltzer
Copy link
Contributor Author

I suppose that since consume is already part of the api, we would need a new name, but perhaps we could mirror the rust api you mentioned and call it into-parts?

That would work for me, but maybe you could remind me why consume is not already a static method?

@elliottt
Copy link
Collaborator

The name evolved over time, and the current behavior of returning an option<incoming-body> that only returns some once was due to wanting to persist access to the other fields of incoming-request while also transferring ownership of the body. Something like into-parts would be a good way to avoid that problem, as we wouldn't be losing access to anything by consuming the original incoming-request.

@acfoltzer
Copy link
Contributor Author

In that case, maybe we should add into-parts as a static method and consider deprecating consume? I'm not sure what standards of compatibility we want to keep between 0.2.0 and 0.3.0 but we could probably remove consume at that point

@elliottt
Copy link
Collaborator

Adding a new static method would be the best path forward for an 0.2.x change, and we can remove or change consume in 0.3+.

acfoltzer added a commit to acfoltzer/wasi-http that referenced this issue Feb 17, 2024
@acfoltzer
Add static into-parts for incoming messages; deprecate consume
2c26e49 
Closes WebAssembly#102. See discussion in that issue for the motivation and design of this change.
@acfoltzer acfoltzer linked a pull request Feb 17, 2024 that will close this issue
Open
@acfoltzer
Copy link
Contributor Author

@elliottt I can't tag you as reviewer, but I just opened #103 with these new static methods.

acfoltzer added a commit to acfoltzer/wasi-http that referenced this issue Feb 21, 2024
@acfoltzer
Add take-headers to get owned headers from incoming messages
55a4754 
Closes WebAssembly#102. See discussion in that issue for the motivation and design of this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@elliottt @acfoltzer @lukewagner