GET request body failing to parse

[jonathannorris]

With an API we are building on Cloudflare Workers we support setting a body on a GET request. It works when it's deployed on a Cloudflare Worker, however we don't get any response from await event.request.json() with miniflare.

Thanks for building this awesome tool!

[SupremeTechnopriest]

Im seeing the same issue with response body:

Example

async function getText (url) {
  const response = await fetch(url)
  const clone = response.clone()
  const text = await clone.text()
  // Hangs
}

getText('https://google.com').then(console.log).catch(console.error)

[mrbbot]

Hey! 👋

@jonathannorris, I'm pretty confused what you're trying to do here. GET or HEAD requests cannot have bodies. When I try run the following code on a Cloudflare Worker I get a TypeError: Request with a GET or HEAD method cannot have a body.:

await fetch("https://httpbin.org/get", {
  method: "GET",
  body: "body",
});

Would you be able to explain your scenario a little more?

---

@SupremeTechnopriest, if I run your code with Miniflare, but add a return text; where the // Hangs comment is, I see the text contents of https://google.com logged to the console. 😕 What version of Miniflare and Node are you using?

[SupremeTechnopriest]

@mrbbot Thanks for getting back to me!

Im using miniflare 1.4.1 and node 14.15.0. Node 14 is Active LTS should I try running it on Latest v16?

The actual code is a bit more complicated. The example I provided is just a minimal reproduction. Here is the actual code:

export async function rewriteContent (request, response) {
  const type = response.headers.get('Content-Type')
  if (!type) return response

  // Rewrite html content
  if (type.startsWith('text/html')) {
    const clone = response.clone()
    const text = await clone.text()
    
    // Requests hangs here.  
    // The promise never resolves and the request never times out.

    const amp = isAmp(text)
    const rewriter = createRewriter(config, amp)
    response = await rewriter.transform(response)
  }
 
  // Rewrite other types (omitted)

  return response
}

[mrbbot]

If you could try it out on v16 that would be great. 🙂 I'll have a bit more of a think on what's causing this...

[SupremeTechnopriest]

@mrbbot Tried it on node 16 and it's the same thing. I have narrowed it down to the cloned response. If I get the text of the original response it works perfectly, but if I try to get the text from the clone it never resolves.

[SupremeTechnopriest]

@mrbbot were you able to reproduce this on your end?

[mrbbot]

@SupremeTechnopriest no, I haven't been able to reproduce this. I have a potential workaround which I'll try tomorrow and report back to you 👍

[SupremeTechnopriest]

Appreciate it!

[mrbbot]

Ok, here it is. It would be great to check if this issue still occurs with undici's fetch implementation. Here's a script that uses Miniflare to replace the fetch and related classes in the sandbox with undici's:

// runner.mjs
import { ConsoleLog, Miniflare } from "miniflare";
import { fetch, Headers, Response, Request, FormData } from "undici";

Headers.prototype.raw = function () {
  return Object.fromEntries(
    [...this.entries()].map(([key, value]) => [key, [value]])
  );
};

const mf = new Miniflare({
  scriptPath: "src/index.js",
  log: new ConsoleLog(true),
  bindings: {
    fetch,
    Headers,
    Response,
    Request,
    FormData,
  },
});
mf.createServer().listen(8787, () => {
  mf.log.info("Listening on :8787");
});

With a recent version of Node 16, run npm install undici --save-dev in your project and then node runner.mjs to start the Miniflare server. You won't be able to make WebSocket requests using fetch, but HTMLRewriter should still work.

Whilst you could use this for developing workers, I wouldn't suggest it. This is more to check whether using undici fixes this and therefore that this issue will be resolved in version 2.

Note that if you are returning the result of a fetch directly, you'll need to wrap it in a new Response, as Miniflare 1 expects mutable Response headers:

addEventListener("fetch", (event) => {
  event.respondWith(handleRequest());
});

async function handleRequest() {
  const res = await fetch("...");
  return new Response(res.body, res);
}

This will only work for format = "service-worker" workers too.

[SupremeTechnopriest]

@mrbbot I will give this a go tomorrow and let you know how it works out. Whats the ETA on v2?

[mrbbot]

The full release will probably be in a month or so, but hopefully a beta will be ready before then.

[SupremeTechnopriest]

@mrbbot That definitely fixes the clone issue, but some other issues crop up with sub requests getting a 520. Im not 100% convinced that its a problem with miniflare yet, but Im going to continue investigating and I'll get back to you. Looking forward to v2. Let me know when a beta is ready!

[SupremeTechnopriest]

Hey @mrbbot,

Been going through this and it seems that when I have caching enabled some sub requests start failing. Im getting the error fetch failed. The cache objects have some odd names. Im not sure if this is by design, but this is what it looks like:

This is with the undici fetch bindings. Turning cache off makes most of these requests start working, but a few subrequests are still failing with the same error. The worker is functioning properly in production at cloudflare.

Just thought I'd bring it to your attention. I feel like we are almost there.

[mrbbot]

Ah whoops, I forgot I do an instanceof check for Request when normalising cache keys:

miniflare/src/kv/cache.ts

Line 19 in 31d0579

return req instanceof Request ? req : new Request(req);

...so cache won't work when you're using this undici-hack.

When you're getting fetch failed, is there any additional information? Are you catching the error somewhere? If so, I think undici attaches a cause property to some of its errors, which might shed some light on what's going on?

[SupremeTechnopriest]

@mrbbot I caught the error and found the cause.

cause: InvalidArgumentError: invalid connection header

I am mutating the Host header, but I'm also mutating it in other requests as well that don't throw. Cloudflare workers allow you to edit the host header as long as it's the same host as the request url. Any ideas?

[dan-lee]

Maybe this is related, I am using ky in my worker:

// this does not work (hangs forever):
await ky.get('/api').json()

// this works:
const response = await ky.get('/api')
const result = await response.json()

Digging further I found that internally they call (await result).clone() before calling .json

[SupremeTechnopriest]

@dan-lee Yeah its the same bug with cloned response. Try shimming with:

Ok, here it is. It would be great to check if this issue still occurs with undici's fetch implementation. Here's a script that uses Miniflare to replace the fetch and related classes in the sandbox with undici's:

// runner.mjs
import { ConsoleLog, Miniflare } from "miniflare";
import { fetch, Headers, Response, Request, FormData } from "undici";

Headers.prototype.raw = function () {
  return Object.fromEntries(
    [...this.entries()].map(([key, value]) => [key, [value]])
  );
};

const mf = new Miniflare({
  scriptPath: "src/index.js",
  log: new ConsoleLog(true),
  bindings: {
    fetch,
    Headers,
    Response,
    Request,
    FormData,
  },
});
mf.createServer().listen(8787, () => {
  mf.log.info("Listening on :8787");
});

With a recent version of Node 16, run npm install undici --save-dev in your project and then node runner.mjs to start the Miniflare server. You won't be able to make WebSocket requests using fetch, but HTMLRewriter should still work.

Whilst you could use this for developing workers, I wouldn't suggest it. This is more to check whether using undici fixes this and therefore that this issue will be resolved in version 2.

Note that if you are returning the result of a fetch directly, you'll need to wrap it in a new Response, as Miniflare 1 expects mutable Response headers:

addEventListener("fetch", (event) => {
  event.respondWith(handleRequest());
});

async function handleRequest() {
  const res = await fetch("...");
  return new Response(res.body, res);
}

This will only work for format = "service-worker" workers too.

[dan-lee]

Thanks for hinting at that @SupremeTechnopriest. Then I get another problem:

Failed to construct 'Request': The provided value 'undefined' is not a valid enum value of type RequestCredentials.

There's no more info or stack trace, not sure where this is coming from.

[SupremeTechnopriest]

@dan-lee Hmmm, to me that means that ky isnt setting a credential field and undici requires it to be explicitly set... Have you tested this worker at cloudflare?

[dan-lee]

@SupremeTechnopriest Yes, this worker is already in production. It was used with wrangler dev before and just recently switched to miniflare.

[SupremeTechnopriest]

Yep me too.

Using vanilla fetch I don't have to manually set the credentials field for undici to work... So the problem is likely with the ky wrapper. Seeing that your code works at cloudflare, it could be a bad interaction between ky and undici.

Is it possible you are setting credentials on the request object to undefined by accident?

[dan-lee]

Wow! Thanks, @SupremeTechnopriest it didn't occur to me that this could actually be the case 🤦 This must be a remnant from earlier development.
It seems to work with undici then!

PS: Some node warnings I get, which might be very well expected

(node:28046) ExperimentalWarning: stream/web is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:28046) ExperimentalWarning: buffer.Blob is an experimental feature. This feature could change at any time

[SupremeTechnopriest]

@dan-lee Yeah I think those are to be expected and safe to ignore. Glad we figured it out!

[SupremeTechnopriest]

@mrbbot Any thoughts on this one? Im going to start familiarizing myself with the miniflare codebase so I can help out where I can.

@mrbbot I caught the error and found the cause.

cause: InvalidArgumentError: invalid connection header

I am mutating the Host header, but I'm also mutating it in other requests as well that don't throw. Cloudflare workers allow you to edit the host header as long as it's the same host as the request url. Any ideas?

[mrbbot]

No, I'm not sure what's going on here. If I think of anything I'll let you know. Just a heads up too, the codebase is currently going through a major restructure for v2.

[SupremeTechnopriest]

Thanks for heads up!

[mrbbot]

Looks like undici doesn't accept requests with Transfer-Encoding, Connection, Keep-Alive or Expect headers. I'm assuming you're fetching the incoming request (proxying) in your worker. Miniflare 2 will strip these headers from the incoming headers so hopefully this should be fixed. 🤞

[SupremeTechnopriest]

@mrbbot Awesome! Looking forward to it. Nice catch!

[mrbbot]

Hey! 👋 The first pre-release of Miniflare 2 has just been released, using undici for its fetch implementation. You can find the full changelog here and install it with npm i miniflare@next -D. Please let me know if you have any other issues, and feel free to ask questions in the #miniflare channel of the Cloudflare Workers Discord server.

[SupremeTechnopriest]

@mrbbot Awesome! In testing it out, I'm getting an error decoding brotli responses. The browser is throwing Failed to load resource: net::ERR_CONTENT_DECODING_FAILED. The rest of the response looks good to me though. All my headers are there. Do you want me to open another issue for brotli encoded responses? Or should we reopen this one?

[SupremeTechnopriest]

This is with cache disabled btw. I tried enabling cache and the file names look good now. Just cant decode the response in the browser.

[mrbbot]

@SupremeTechnopriest Let's open a new issue. 👍