bytecodealliance / ComponentizeJS

JS -> WebAssembly Component
Apache License 2.0
242 stars 32 forks source link

Use of `wasi-http` fails to fully finish writing the body #99

Closed brooksmtownsend closed 7 months ago

brooksmtownsend commented 7 months ago

I've been trying to replicate a few examples using ComponentizeJS with TypeScript and I've been trouble using a component that uses wasi-http. Here's the component code:

import {
  IncomingRequest,
  ResponseOutparam,
  OutgoingResponse,
  OutgoingBody,
  OutputStream,
  Fields,
} from "wasi:http/types@0.2.0";

// Implementation of wasi-http incoming-handler
function handle(req: IncomingRequest, resp: ResponseOutparam) {
  // Start building an outgoing response
  const outgoingResponse = new OutgoingResponse(new Fields());

  // Set the status code for the response
  outgoingResponse.setStatusCode(200);
  // Access the outgoing response body
  const outgoingBody = outgoingResponse.body();
  // Create a stream for the response body
  let outputStream: OutputStream | null = outgoingBody.write();
  // Write hello world to the response stream
  outputStream.blockingWriteAndFlush(
    new Uint8Array(new TextEncoder().encode("Hello from Typescript!\n"))
  );

  // Set the created response
  ResponseOutparam.set(resp, { tag: "ok", val: outgoingResponse });
}

export const incomingHandler = {
  handle,
};

Now when I use this and curl the endpoint, I do get "Hello from Typescript", but the component hangs indefinitely. After taking a look at a couple other examples I noticed that I have one line missing here:

  outputStream.blockingWriteAndFlush(
    new Uint8Array(new TextEncoder().encode("Hello from Typescript!\n"))
  );
  ResponseOutparam.set(resp, { tag: "ok", val: outgoingResponse });
  OutgoingBody.finish(outgoingBody, undefined); // This line, to "finish" the outgoing body

However trying to run this gets me an issue:

➜ wasmtime serve -Scommon ./build/http_hello_world_s.wasm
Serving HTTP on http://0.0.0.0:8080/
2024-04-10T16:15:42.362242Z ERROR wasmtime_cli::commands::serve: [0] :: Error {
    context: "error while executing at wasm backtrace:\n    0: 0xa71792 - wit-component:shim!indirect-wasi:http/types@0.2.0-[static]outgoing-body.finish\n    1: 0x73cd95 - <unknown>!<wasm function 8453>\n    2: 0x3360a9 - <unknown>!<wasm function 515>\n    3: 0xc777a - <unknown>!<wasm function 104>\n    4: 0x33386d - <unknown>!<wasm function 511>\n    5: 0x335f04 - <unknown>!<wasm function 515>\n    6: 0x67ed71 - <unknown>!<wasm function 4646>\n    7: 0x4de593 - <unknown>!<wasm function 1620>\n    8: 0x3c161e - <unknown>!<wasm function 761>\n    9: 0x76e374 - <unknown>!wasi:http/incoming-handler@0.2.0#handle",
    source: HasChildren,
}   

Do you know how to compensate for this error? In Rust and with componentize-py this code works 🤔 In tinygo, I did end up needing to explicitly drop the outputStream equivalent but I don't see a way to do this in JS/TS

brooksmtownsend commented 7 months ago

For easier testing, here is the raw JS:

import { ResponseOutparam, OutgoingResponse, OutgoingBody, Fields, } from "wasi:http/types@0.2.0";
// Implementation of wasi-http incoming-handler
//
// NOTE: To understand the types involved, take a look at wit/deps/http/types.wit
function handle(req, resp) {
    // Start building an outgoing response
    const outgoingResponse = new OutgoingResponse(new Fields());
    // Set the status code for the response
    outgoingResponse.setStatusCode(200);
    // Access the outgoing response body
    const outgoingBody = outgoingResponse.body();
    // Create a stream for the response body
    let outputStream = outgoingBody.write();
    // Write hello world to the response stream
    outputStream.blockingWriteAndFlush(new Uint8Array(new TextEncoder().encode("Hello from Typescript!\n")));
    // Set the created response
    ResponseOutparam.set(resp, { tag: "ok", val: outgoingResponse });
    OutgoingBody.finish(outgoingBody, undefined);
}
export const incomingHandler = {
    handle,
};
brooksmtownsend commented 7 months ago

Just a note that I tried to recreate https://github.com/bytecodealliance/jco/blob/7b41c3551d26840a65682803e2c64810645ad732/test/fixtures/componentize/wasi-http-proxy/source.js#L32, and I see the comment there for TODO which might be causing the failure? Unsure

guybedford commented 7 months ago

Yes, I believe you need to explicitly drop for WASI to work here - outputStream[Symbol.dispose]().

There's also an example of this in https://github.com/bytecodealliance/ComponentizeJS/blob/main/test/cases/http-request/source.js.

With the explicit resource management proposal (https://github.com/tc39/proposal-explicit-resource-management), and the enforcement of using being presented at TC39 tomorrow, this can be fully solved in future bindgen approaches.

But for now, yes it's a major footgun.

brooksmtownsend commented 7 months ago

Hm I'm surprised, that kept the same behavior 🤔

import { ResponseOutparam, OutgoingResponse, Fields, } from "wasi:http/types@0.2.0";
// Implementation of wasi-http incoming-handler
//
// NOTE: To understand the types involved, take a look at wit/deps/http/types.wit
function handle(req, resp) {
    // Start building an outgoing response
    const outgoingResponse = new OutgoingResponse(new Fields());
    // Access the outgoing response body
    let outgoingBody = outgoingResponse.body();
    {
        // Create a stream for the response body
        let outputStream = outgoingBody.write();
        // Write hello world to the response stream
        outputStream.blockingWriteAndFlush(new Uint8Array(new TextEncoder().encode("Hello from Typescript!\n")));
        // @ts-ignore: This is required in order to dispose the stream before we return
        outputStream[Symbol.dispose]();
    }
    // Set the status code for the response
    outgoingResponse.setStatusCode(200);
    // Set the created response
    ResponseOutparam.set(resp, { tag: "ok", val: outgoingResponse });
}
export const incomingHandler = {
    handle,
};

I had to tell TS to ignore the warning but that's not a big deal. I'm glad to hear that TC39 will talk about this 🙂

guybedford commented 7 months ago

Great, does that resolve the issue then for now at least?

brooksmtownsend commented 7 months ago

@guybedford No it's the same behavior 😢 It didn't seem to affect the problem.

guybedford commented 7 months ago

Oh, you need to call OutgoingBody.finish(outgoingBody) where you import the OutgoingBody in the import as it's a static method.

guybedford commented 7 months ago

Also, if you want to actually stream the response, connect the stream by calling ResponseOutparam.set(resp, { tag: "ok", val: outgoingResponse }); earlier, before the stream is written to.

brooksmtownsend commented 7 months ago

Calling OutgoingBody.finish(outgoingBody, undefined) was what I needed!

brooksmtownsend commented 7 months ago

@guybedford just wanted to note that I'm gonna close this since my issue is resolved, but let me know if I can assist with updating the bindings to do this automatically. It feels generally like logic that is specific to resources and not ComponentizeJS, so it's implemented correctly here.

guybedford commented 7 months ago

@brooksmtownsend makes sense, yes it is leaning entirely on the bindings model here. But also, in terms of things we might do to catch these types of issues:

  1. For resources which require to be "explicitly dropped", we could provide a warning on the JS GC that warns that the resource was not explicitly dropped, and then at the end of the application lifecycle, provide these warnings.
  2. For resources which require to be "explicitly consumed", we could have some kind of metadata which notes this and then instead of giving the warning above, instead give a warning with a suggestion of the consuming API method to use.

It's just a little bit of metadata but it could make the user experience a bunch better.