[v0.3]: Changes to `consume-body`

[alexcrichton]

There was some discussion today about consume-body, possible changes, and consequences of various design decisions. I wanted to write down what I recall are the results of the discussion to make sure they don't get lost, but @rvolosatovs, @lukewagner, @lann, or @dicej correct me if I'm wrong.

The discussion started with the problem of: the consume-body semantics of today are surprisingly heavyweight to implement, notably the "after you close the handles handed out you can call consume-body again" part. We concluded that this should be removed to make consume-body callable at most one time.

The discussion then veered into how there's also problems around implementing the future returned from request/response constructors right now. For example it's not clear how to resolve those futures when the request/response is created in a guest and then passed to another guest. What sort of needs to happen is that the writer is owned by the request or response object (e.g. on the host) and somehow that needs to be written to, but with the current API it's not clear when.

To resolve this we ended up concluding that consume-body should take a future<result<_, error-code>> as an argument. This is a guest's indication of "I'm taking the body, and the result of my processing the body will be this future". The future is then "fused" with the write end already present in the response or request, completing the chain of how to resolve it.

There was a lot of other discussion and alternatives I'm not capturing here for now, too. There's also some various edge cases we probably still need to worry about here (e.g. should the future argument be optional for performance today?) which may still need hammering out.

[rvolosatovs]

In the common use case of a host embedding calling a guest exporting a handle, this future then becomes the (only) way for the guest to signal an error to the host occurring after handle has returned.
That also then means that the host should only ever await the read end of this future after handle has returned.
Does that sound correct?

[alexcrichton]

That sounds reasonable to me yeah, although I'd also defer to others

[lukewagner]

@rvolosatovs I might be misunderstanding the question, but my impression is that a guest exporting a handle function called by the host could start consuming the given request in the guest, see an error, then future.write to the future passed to consume-body, all before returning a response to the host. In this case, it could be useful for the host to immediately receive and propagate this error (without the host first waiting for handle to return a response).

[lann]

It seems like it could meaningfully simplify things if we pulled the body/trailers state machine out of request/response, e.g.:

interface types {
  resource request {
    constructor(headers: headers);
    ...
  }
  resource response {
    constructor(headers: headers);
    ...
  }
}

interface handler {
  // Resolves when sending a message (including body + trailers) is complete
  type send-complete = future<result<option<trailers>, error-code>>;
  // Resolves when receiving a message (including body + trailers) is complete
  type recv-complete = future<result<_, error-code>>

  record request-parts {
    request: request,
    body: option<stream<u8>>,
    request-send-complete: send-complete,
    response-recv-complete: recv-complete,
    options: option<request-options>,  
  }
 
  record response-parts {
    response: response,
    body: stream<u8>,
    response-send-complete: send-complete,
    request-recv-complete: recv-complete,
  }

  // Resolves when the response headers are received
  handle: async func(request-parts) -> result<response-parts, error-code>;
}

edit: updated in response to feedback below

[lukewagner]

@lann That's a good thought, but I think there are two remaining challenges:

First: in addition to the {request,response}-complete future that flows in the same direction as the stream<u8> (as the final value after the last u8), you also need a second future<result<...>> that flows in the opposite direction of the stream (indicating the final success/failure (and HTTP-specific failure code) of the writes to the stream); this reverse in direction could be achieved by the consume-result parameter in:

  consume-body: func(
    consume-result: future<result<...>>
  ) -> result<tuple<stream<u8>, future<result<...>>>;

(If we had a writable-future<T>, then consume-result could instead be a 3rd field of the returned tuple, but if we're going to add something significant to better solve this use case, I think the better solution is stream<u8, result<trailers, error-code>, result<_, error-code>> which would mean: N u8s followed by 1 result<trailers, error-code> followed by 1 result<_, error-code> flowing in the opposite direction. I think this generalized stream<T,U,V> is what you need to describe many kinds of network-bound or storage-bound streams.)

Second: I think wasi-http needs to enforce Content-Length and, without a function like request.new to bind the readable end of a stream to a now-immutable headers, I don't know how we do that in a general component-to-component setting.

[lann]

you also need a second future<result<...>> that flows in the opposite direction

Ah yes of course. I updated the comment above to add that and updated various names for clarity.

stream<T,U,V> is what you need

Completely agreed; my suggestion is trying to emulate that as closely as possible with what we currently have.

[lann]

Second: I think wasi-http needs to enforce Content-Length and, without a function like request.new to bind the readable end of a stream to a now-immutable headers, I don't know how we do that in a general component-to-component setting.

I'm not sure I follow here; can't an implementation of handle enforce content-length just as easily as an implementation of request.new?

[lukewagner]

Ah yes of course. I updated the comment above to add that and updated various names for clarity.

This mostly achieves the same effect as consume-body, but one difference is that it couples the receipt of the *-recv-complete future with receiving the complementary response/request. E.g., if I send an outgoing request via imported handle, the entire sending of the request may complete or error long before I receive the first bytes of the response and thus before handle returns. More generally, it seems useful for request and response to be decoupled, fully-independent "streams plus extra metadata before+after" resources that can be passed around independently (e.g., you could imagine some authenticate: func(in: request) -> result<request, error> or decompress: func(in: response) -> result<response, error> and these sorts of functions would be trickier with the "broken into parts" approach.

I'm not sure I follow here; can't an implementation of handle enforce content-length just as easily as an implementation of request.new?

If we're chaining components (by having one component's exported handle be another component's imported handle, then that means each component must defensively enforce Content-Length which basically means that it's not enforced by wasi-http (even though hosts naturally would, which means you'd have components that can get away with the assumption when tested in isolation, but then fail when composed). In contrast, wasi:http/types is not expected to be implemented by most components (and when it is, it's for some intense virtualization scenario where you'd expect to have to enforce all the wasi-http rules anyways).

[lann]

I think there is probably a fractal of potential discussions here so I'll try to rewind to my actual root concern: in my experience wasi:http 0.2 has been difficult to implement correctly, in part because it is difficult to keep track of the concurrent state machine between e.g. input-stream.read, incoming-body.finish, future-trailers.get, output-stream.write, outgoing-body.finish. I had hoped that async abstractions would improve this situation but it feels like there is even more to keep track of in 0.3.

Undoubtedly some of this can be chalked up to my own unfamiliarity with the interface design (process) but I'm not sure that I'll ever be able to keep the exact semantics of consume-body: func(future<result<_, error-code>>) -> result<tuple<stream<u8>, future<result<option<trailers>, error-code>>>> in my head for very long. 🙂

[rvolosatovs]

I've opened #185 with the proposed change and a small addition of move semantics to consume-body.
Wasmtime implementation in bytecodealliance/wasmtime#11653

Please review!