Overhaul fetch implementation and host API

[tschneidereit]

This PR contains a substantial overhaul of the fetch implementation, along with meaningful changes to how the host API works

On the latter, the most important change is that the representation of handles is now completely opaque to consumers: instead of using a typedef visible to users of the host_api.h header, the header just forward-declares a class, which hence can't be created or interacted with from outside the host API implementation.

[tschneidereit]

@guybedford while this isn't completely done, and in dire need of cleanup, it is most of the way there, I think. I'm sharing it now because the host API changes are bound to impact you quite a bit. I hope they're for the better, and I at least found them to be necessary to get back to a point where I'm able to reason through the management and lifecycles of resource handles.

[tschneidereit]

@guybedford I think this is now ready for review. The resource handling is much cleaned up, and I addressed all the regressions of WPT tests that make sense to do here, relative to the state of the JS Compute Runtime I started with.

(The only things missing that that passes are around some convenience constructors such as Response.redirect(), which I'm not tackling here.)

[nicoburniske]

Hey @tschneidereit, I tried out your branch with some e2e tests that I created in my branch. There looks to be a problem with attaching a body to a fetch request

The error I encountered was

stderr [0] :: event loop error - both task and job queues are empty, but expected operations did not resolve
stderr [0] :: Internal error.

Here's the full e2e test code. I can put up a branch if you'd like

async function main(event) {
    let resolve, reject;
    
    try {
        let responsePromise = new Promise(async (res, rej) => {
            resolve = res;
            reject = rej;
        });
        event.respondWith(responsePromise);

        let body = `{ "hello": "world" }`
        
        // Error occurs here
        let response = await fetch("https://echo.free.beeceptor.com", {
            method: "POST",
            headers: {
                "Content-Type": "application/json"
            },
            body
        })

        // This never logs
        console.log("Successfully sent request to echo server")

        let responseJson = await response.json();
        
        let result = {
            method: responseJson.method,
            parsedBody: responseJson.parsedBody,
        };

        console.log("Successfully received response json body");
        resolve(new Response(JSON.stringify(result)));
    } catch (e) {
        console.log(`Error: ${e}. Stack: ${e.stack}`);
    }
}

addEventListener('fetch', main);

[tschneidereit]

Thank you for letting me know, @nicoburniske! Fascinating, given that I ran a whole bunch of WPT tests around fetch that seemed to say it works. I'll investigate.

[tschneidereit]

@nicoburniske thank you again for the report! Turns out I had read some of the WPT outputs wrong, and there indeed was a bunch more stuff to fix 😃

[nicoburniske]

@tschneidereit Happy to help! Excited for these fixes to ship 🚀

[guybedford]

I briefly tested this out in ComponentizeJS and can confirm fetch seems to behave correctly, down to some possible ComponentizeJS cases to work through.

[guybedford]

One issue I did hit with this example:

const res = await fetch('https://www.google.com');
const text = await res.text();
console.log(text);

is that it throws (new TypeError("malformed UTF-8 character sequence at offset 1", ""))

It could be worth verifying decompression is properly supported, otherwise this may be a ComponentizeJS / Jco bug.

[tschneidereit]

@guybedford ah, interesting. The fetch implementation indeed doesn't do transparent decompression. But usually that shouldn't matter either, because we're not setting the Accept-Encoding header. Are you setting that, by any chance?

[guybedford]

@tschneidereit because we virtualize on top of Node.js's fetch implementation it might be overriding that I expect, posted bytecodealliance/jco#447 to investigate further.

[guybedford]

Actually I spoke too soon. Running against a test server I'm getting mangled response body data. Headers are just {"host":"localhost:8080","connection":"keep-alive"}. But the body the test server is sending back doesn't seem to reflect at all. Again, this could be ComponentizeJS / Jco toolchain specific, but I'd have to dig into it more. We did pass all the WASI HTTP tests though, so I haven't seen any mangled response issues before, unless there are gaps in the test suite.

[tschneidereit]

@guybedford I fixed a whole bunch of additional issues now. Would be great if you could take a look at the additional commits I pushed since your review. Then we should land this, and move everything else into additional PRs. (Of which I'll open a few in a moment, stacked on top of this one 😄)

[guybedford]

I'm still getting those weird issues with streaming when testing this in ComponentizeJS. I don't know if they are ComponentizeJS or Jco bugs though. I'm assuming streaming res.text() is properly captured in the WPT tests though? If so, what might be necessary to properly debug these cases is to try running the WPT using the Jco host instead of Wasmtime, to confirm the scenarios, before investigating if this is a ComponentizeJS issue, raising any upstream issue as necessary.

I have also tested this on Fastly's StarlingMonkey port and can confirm all still works correctly.

Would be great to land this now though and continue iterating with follow-ups as necessary.

[guybedford]

What case is this branch for?

[tschneidereit]

I extracted this out into a function with a short doc comment explaining the purpose.

[tschneidereit]

Thank you for the review! I pushed a commit addressing your comments. While there's a bunch of code motion in that commit, it's all trivial, so I'll go ahead and land this.