Skip to content
This repository was archived by the owner on Nov 25, 2025. It is now read-only.

[v0.3] model bodies after stream<u8, trailers, error-code>, trap less #162

Merged
lukewagner merged 11 commits into from
Apr 11, 2025
Merged

[v0.3] model bodies after stream<u8, trailers, error-code>, trap less #162

Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
aba4ece
feat(p3): be less trappy
rvolosatovs Mar 13, 2025
1b2455f
chore(p3): orphan `headers` returned by `headers()`
rvolosatovs Mar 13, 2025
128af94
doc(p3): improve wording
rvolosatovs Mar 19, 2025
55db2ff
feat(p3): avoid auto-close semantics
rvolosatovs Mar 19, 2025
70ad39c
p3: allow multiple active body refs to exist, drop `into-parts`
rvolosatovs Mar 20, 2025
eac8416
p3: add `request-options#clone`
rvolosatovs Mar 20, 2025
f927cf4
p3: model `body` after `stream<u8, trailers, error-code>`
rvolosatovs Mar 19, 2025
865daa5
p3: remove `body` resource
rvolosatovs Mar 21, 2025
c3517c6
p3: use `result`, not `option` in `body()`
rvolosatovs Mar 21, 2025
ae89575
p3: make request/response bodies optional
rvolosatovs Mar 24, 2025
848eaf5
p3: document optional content stream optimization
rvolosatovs Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 73 additions & 93 deletions wit-0.3.0-draft/types.wit
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,77 +230,35 @@ interface types {
/// Trailers is an alias for Fields.
type trailers = fields;

/// Represents an HTTP Request or Response's Body.
///
/// A body has both its contents - a stream of bytes - and a (possibly empty)
/// set of trailers, indicating that the full contents of the body have been
/// received. This resource represents the contents as a `stream<u8>` and the
/// delivery of trailers as a `trailers`, and ensures that the user of this
/// interface may only be consuming either the body contents or waiting on
/// trailers at any given time.
resource body {

/// Construct a new `body` with the specified stream.
///
/// This function returns a future, which will resolve
/// to an error code if transmitting stream data fails.
///
/// The returned future resolves to success once body stream
/// is fully transmitted.
new: static func(
%stream: stream<u8>,
) -> tuple<body, future<result<_, error-code>>>;

/// Construct a new `body` with the specified stream and trailers.
///
/// This function returns a future, which will resolve
/// to an error code if transmitting stream data or trailers fails.
///
/// The returned future resolves to success once body stream and trailers
/// are fully transmitted.
new-with-trailers: static func(
%stream: stream<u8>,
trailers: future<trailers>
) -> tuple<body, future<result<_, error-code>>>;

/// Returns the contents of the body, as a stream of bytes.
///
/// This function may be called multiple times as long as any `stream`s
/// returned by previous calls have been dropped first.
///
/// On success, this function returns a stream and a future, which will resolve
/// to an error code if receiving data from stream fails.
/// The returned future resolves to success if body is closed.
%stream: func() -> result<tuple<stream<u8>, future<result<_, error-code>>>>;

/// Takes ownership of `body`, and returns an unresolved optional `trailers` result.
///
/// This function will trap if a `stream` child is still alive.
finish: static func(this: body) -> future<result<option<trailers>, error-code>>;
}

/// Represents an HTTP Request.
resource request {

/// Construct a new `request` with a default `method` of `GET`, and
/// `none` values for `path-with-query`, `scheme`, and `authority`.
///
/// * `headers` is the HTTP Headers for the Response.
/// * `body` is the optional contents of the body, possibly including
/// trailers.
/// * `options` is optional `request-options` to be used if the request is
/// sent over a network connection.
/// `headers` is the HTTP Headers for the Request.
///
/// `contents` is the body content stream. Once it is closed,
/// `trailers` future must resolve to a result.
/// If `trailers` resolves to an error, underlying connection
/// will be closed immediately.
///
/// `options` is optional `request-options` resource to be used
/// if the request is sent over a network connection.
///
/// It is possible to construct, or manipulate with the accessor functions
/// below, an `request` with an invalid combination of `scheme`
/// below, a `request` with an invalid combination of `scheme`
/// and `authority`, or `headers` which are not permitted to be sent.
/// It is the obligation of the `handler.handle` implementation
/// to reject invalid constructions of `request`.
constructor(
///
/// The returned future resolves to result of transmission of this request.
new: static func(
headers: headers,
body: option<body>,
contents: stream<u8>,
Copy link
Copy Markdown
Member

@tschneidereit tschneidereit Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs to be optional, since not all requests have bodies, right?

Copy link
Copy Markdown
Contributor Author

@rvolosatovs rvolosatovs Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we discussed with @lukewagner and @dicej that body resource would be non-optional (#165), that's something that would have addressed #164.

In that scenario requests not carrying a body (like GET) would be represented by empty streams.

Now that there's no body resource anymore and it's the request/response constructors that return the transmit future, we can reconsider this, indeed, especially since the host would not be able to do anything with the trailers future in that case anyway.

body parameter will have to become an option<tuple<..>> though

Copy link
Copy Markdown
Member

@tschneidereit tschneidereit Mar 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation; makes sense to me

trailers: future<result<option<trailers>, error-code>>,
Copy link
Copy Markdown
Member

@tschneidereit tschneidereit Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thought on this: are there downsides to always having to pass a future here? I'm not sure what the overhead (both resource- and boilerplate-wise) of creating an managing a future is. If it's trivial, then this doesn't much matter, but otherwise, should we consider wrapping this into an option<>?

Copy link
Copy Markdown
Contributor Author

@rvolosatovs rvolosatovs Mar 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the "stream<T, U, V> world", if we go with optional body in constructors, the parameter would be option<stream<u8, option<trailers>, error-code>>, meaning that users could either pass no body stream or pass a body stream, with potentially empty contents, but mandatory final element (option<trailers> or error-code).
From what I understand, there would not be a way to optionally pass a final stream element.

With that in mind, if we want to make the future optional, we'd need to take an option<tuple<stream<u8>, future<result<option<trailers>, error-code>>>>, which would then directly translate to option<stream<u8, option<trailers>, error-code>> in the future. (see also #162 (comment))

FWIW, here's what it takes to create a new future in Rust guest and pass to a constructor: https://github.com/bytecodealliance/wasip3-prototyping/blob/ad4ccf03172b22320c4a22971c51b4f7249a2846/crates/test-programs/src/p3/http.rs#L78-L81
and then send a value: https://github.com/bytecodealliance/wasip3-prototyping/blob/ad4ccf03172b22320c4a22971c51b4f7249a2846/crates/test-programs/src/p3/http.rs#L112
So 2 LoC, plus an import of futures::SinkExt to expose futures::SinkExt::send

I would not expect creating a future handle to be an expensive operation, especially considering the fact that we're most likely dealing with network I/O here.

I'm happy to switch to a tuple here, if that's what everyone prefers

Copy link
Copy Markdown
Member

@lukewagner lukewagner Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the overhead shouldn't be too significant. Also, it encourages the generally good engineering practice of explicitly signalling the final success or failure of the request/response in the result.

options: option<request-options>
);
) -> tuple<request, future<result<_, error-code>>>;

/// Get the Method for the Request.
method: func() -> method;
Expand Down Expand Up @@ -348,22 +306,28 @@ interface types {
///
/// The returned `headers` resource is immutable: `set`, `append`, and
/// `delete` operations will fail with `header-error.immutable`.
///
/// This headers resource is a child: it must be dropped before the parent
/// `request` is dropped, or its ownership is transferred to another
/// component by e.g. `handler.handle`.
headers: func() -> headers;

/// Get the body associated with the Request, if any.
/// Get body of the Request.
///
/// This body resource is a child: it must be dropped before the parent
/// `request` is dropped, or its ownership is transferred to another
/// component by e.g. `handler.handle`.
body: func() -> option<body>;

/// Takes ownership of the `request` and returns the `headers`, `body`
/// and `request-options`, if any.
into-parts: static func(this: request) -> tuple<headers, option<body>, option<request-options>>;
/// Stream returned by this method represents the contents of the body.
/// Once the stream is reported as closed, callers should await the returned future
/// to determine whether the body was received successfully.
/// The future will only resolve after the stream is reported as closed.
///
/// The stream and future returned by this method are children:
/// they should be closed or consumed before the parent `response`
/// is dropped, or its ownership is transferred to another component
/// by e.g. `handler.handle`.
///
/// This method may be called multiple times.
///
/// This method will return `none` if it is called while either:
/// - a stream or future returned by a previous call to this method is still open
/// - a stream returned by a previous call to this method has reported itself as closed
/// Thus there will always be at most one readable stream open for a given body.
/// Each subsequent stream picks up where the last stream left off, up until it is finished.
body: func() -> option<tuple<stream<u8>, future<result<option<trailers>, error-code>>>>;
Copy link
Copy Markdown
Member

@tschneidereit tschneidereit Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unsure whether using option for this makes sense: it seems overloaded for request that just don't have a body, and lead to confusing silent failures.

Given that, ISTM the better, if slightly more onerous, signature would be

body: func() -> option<result<tuple<stream<u8>, future<result<option<trailers>, error-code>>>, body-in-use-error>>;

(Where body-in-use-error would be a new thing, where I don't have strong feelings about what it should look like, exactly.)

Copy link
Copy Markdown
Contributor Author

@rvolosatovs rvolosatovs Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're making bodies optional again, that seems to follow, yes.
I'd prefer not adding the body-in-use-error just yet though and keep it at result<tuple<stream<u8>, future<result<option<trailers>, error-code>>>> for now, verify the design with an implementation and potentially revisit later (as that seems to be a minor change)

Copy link
Copy Markdown
Member

@lukewagner lukewagner Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be missing it, but I couldn't find any prohibition against trailers in GET requests, so if there were an option wrapping the stream<u8>, I think it'd need to be inside the tuple. However, I haven't been able to find a semantic distinction between none and "zero-length contents" inside RFC 9110, so I worry that by introducing this distinction, we might be creating something that content may incorrectly depend on. (E.g., let's say a component decides to fail if the option<stream<u8>> is non-none, but now it's rejecting some zero-length stream which maybe it gets passed (in a multi-component composition).).

Considering the perf angle (which could be another reason for the option):

  • As an optimization and convenience, I do think it'd make sense to have the contents parameter to {request,response}.new be option<stream<u8>> as Till suggested, but none could be spec-defined to be normalized to an empty stream internally.
  • I expect guest code simply won't ask for the content stream for GET requests, so source-language stream objects won't be wastefully created. The CABI overhead of creating closed-on-construction streams should also be light, I think.

So perhaps:

body: func() -> result<tuple<stream<u8>, future<result<option<trailers>, error-code>>>>;

?

Copy link
Copy Markdown
Member

@tschneidereit tschneidereit Mar 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch: I hadn't considered that my proposal would prevent access to trailers as well.

With that, I agree with the proposed interface, with one slight exception: I do think this is a categorically different error from the ones represented by error-code, and that we should make it its own type. Is this perhaps even a pattern that'd make sense to lift up higher and use the same error type for in other places as well?

Copy link
Copy Markdown
Contributor Author

@rvolosatovs rvolosatovs Mar 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As currently proposed, the error type in this case is actually unit, e.g. in Rust it's ().
Here's the host implementation with the full Rust type: https://github.com/bytecodealliance/wasip3-prototyping/blob/ecd781215e650dc5eb4e6ec4b2fd60858c084654/crates/wasi-http/src/p3/host/types.rs#L669-L700

Does this address the concern?

Copy link
Copy Markdown
Member

@tschneidereit tschneidereit Mar 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda? It's better than using error-code (sorry for missing that it's not), but it does seem very low-effort to define a body-in-use-error type and return that instead. And would make this much more self-documenting.

Copy link
Copy Markdown
Member

@lukewagner lukewagner Mar 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't seen us use that idiom before in WASI of having singleton error variants; there's usually just a result.

Copy link
Copy Markdown
Member

@tschneidereit tschneidereit Mar 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, you're absolutely right: I had confused myself into thinking that if we move away from error-code, we need a specific replacement for it. But since the result here is only going to be err if the body is already taken out, I agree that that's not needed. Sorry for the noise.

}

/// Parameters for making an HTTP Request. Each of these parameters is
Expand Down Expand Up @@ -400,6 +364,10 @@ interface types {
/// body stream. An error return value indicates that this timeout is not
/// supported or that this handle is immutable.
set-between-bytes-timeout: func(duration: option<duration>) -> result<_, request-options-error>;

/// Make a deep copy of the `request-options`.
/// The resulting `request-options` is mutable.
clone: func() -> request-options;
}

/// This type corresponds to the HTTP standard Status Code.
Expand All @@ -408,17 +376,23 @@ interface types {
/// Represents an HTTP Response.
resource response {

/// Construct an `response`, with a default `status-code` of `200`. If a
/// different `status-code` is needed, it must be set via the
/// Construct a new `response`, with a default `status-code` of `200`.
/// If a different `status-code` is needed, it must be set via the
/// `set-status-code` method.
///
/// * `headers` is the HTTP Headers for the Response.
/// * `body` is the optional contents of the body, possibly including
/// trailers.
constructor(
/// `headers` is the HTTP Headers for the Response.
///
/// `contents` is the body content stream. Once it is closed,
/// `trailers` future must resolve to a result.
/// If `trailers` resolves to an error, underlying connection
/// will be closed immediately.
///
/// The returned future resolves to result of transmission of this response.
new: static func(
headers: headers,
body: option<body>,
);
contents: stream<u8>,
Copy link
Copy Markdown
Member

@tschneidereit tschneidereit Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as for request

trailers: future<result<option<trailers>, error-code>>,
) -> tuple<response, future<result<_, error-code>>>;

/// Get the HTTP Status Code for the Response.
status-code: func() -> status-code;
Expand All @@ -427,25 +401,31 @@ interface types {
/// given is not a valid http status code.
set-status-code: func(status-code: status-code) -> result;

/// Get the headers associated with the Request.
/// Get the headers associated with the Response.
///
/// The returned `headers` resource is immutable: `set`, `append`, and
/// `delete` operations will fail with `header-error.immutable`.
///
/// This headers resource is a child: it must be dropped before the parent
/// `response` is dropped, or its ownership is transferred to another
/// component by e.g. `handler.handle`.
headers: func() -> headers;

/// Get the body associated with the Response, if any.
/// Get body of the Response.
///
/// This body resource is a child: it must be dropped before the parent
/// `response` is dropped, or its ownership is transferred to another
/// component by e.g. `handler.handle`.
body: func() -> option<body>;

/// Takes ownership of the `response` and returns the `headers` and `body`,
/// if any.
into-parts: static func(this: response) -> tuple<headers, option<body>>;
/// Stream returned by this method represents the contents of the body.
/// Once the stream is reported as closed, callers should await the returned future
/// to determine whether the body was received successfully.
/// The future will only resolve after the stream is reported as closed.
///
/// The stream and future returned by this method are children:
/// they should be closed or consumed before the parent `response`
/// is dropped, or its ownership is transferred to another component
/// by e.g. `handler.handle`.
///
/// This method may be called multiple times.
///
/// This method will return `none` if it is called while either:
/// - a stream or future returned by a previous call to this method is still open
/// - a stream returned by a previous call to this method has reported itself as closed
/// Thus there will always be at most one readable stream open for a given body.
/// Each subsequent stream picks up where the last stream left off, up until it is finished.
body: func() -> option<tuple<stream<u8>, future<result<option<trailers>, error-code>>>>;
Copy link
Copy Markdown
Member

@tschneidereit tschneidereit Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as for request

}
}