Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

500 Internal Server Error / on 304 response and empty ReadableStream #165

Closed
lukaszczerpak opened this issue Jan 28, 2022 · 4 comments
Closed
Labels
behaviour mismatch Different behaviour to Workers runtime

Comments

@lukaszczerpak
Copy link

lukaszczerpak commented Jan 28, 2022

It seems there is inconsistency between CF and Miniflare. PoC code:

addEventListener('fetch', function(event) {
  event.respondWith(handleRequest(event.request))
})
async function handleRequest(request) {
  let url = new URL(request.url)
  const res = await fetch(`http://origin.cloudtest.it${url.pathname}`, {headers: {"if-modified-since": "Wed, 12 Jan 2022 10:43:29 GMT"}})
  return new Response(res.body, {
    headers: {
      ...Object.fromEntries(res.headers),
      "x-workers-hello": "Hello from Cloudflare Workers"
    },
    status: res.status,
    statusText: res.statusText
  })
}

Throws the following error:

GET /bigfiles/256m 500 Internal Server Error (98.22ms)
[mf:err] GET /bigfiles/256m: TypeError: Response with null body status cannot have body
    at new Response (/Users/lukes/labs/cf-workers/sandbox-lukasz-testing/node_modules/undici/lib/fetch/response.js:160:15)
    at new Response (/Users/lukes/labs/cf-workers/sandbox-lukasz-testing/node_modules/@miniflare/core/src/standards/http.ts:556:13)
    at handleRequest (/Users/lukes/labs/cf-workers/sandbox-lukasz-testing/index.js:22:10)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at EventTarget.[kDispatchFetch] (/Users/lukes/labs/cf-workers/sandbox-lukasz-testing/node_modules/@miniflare/core/src/standards/event.ts:359:13)
    at Server.<anonymous> (/Users/lukes/labs/cf-workers/sandbox-lukasz-testing/node_modules/@miniflare/http-server/src/index.ts:167:20)

The origin server returns 304 response with no body (as expected). The reason of the error is that res.body is a ReadableStream which is empty but the object itself is not, therefore the undici library doesn't like it and throws an error (304 responses don't have response body).
I was sitting on the fence if this should be reported here or maybe to the dependency's maintainer. In fact, the object type could be detected to behave properly. WDYT?

@mrbbot
Copy link
Contributor

mrbbot commented Jan 29, 2022

Hey! 👋 Thanks for raising this. This is the right place to report the issue. Executing this code in Chrome/Firefox/undici gives me a ReadableStream body, whereas Cloudflare gives null:

const  res = await fetch("https://httpbin.org/cache", {
  headers: {
    "If-None-Match": "etag"
  }
});
console.log(res.body);

I guess the solution here is to just return null from the res.body getter whenever a null-body status is being used?

@mrbbot mrbbot added the behaviour mismatch Different behaviour to Workers runtime label Jan 29, 2022
@lukaszczerpak
Copy link
Author

lukaszczerpak commented Jan 31, 2022

imho the following change should do the job:

diff --git a/packages/core/src/standards/http.ts b/packages/core/src/standards/http.ts
index bd59d92..aef6da8 100644
--- a/packages/core/src/standards/http.ts
+++ b/packages/core/src/standards/http.ts
@@ -551,7 +551,7 @@ export class Response<
         // This zero-length body behavior is allowed because it was previously
         // the only way to construct a Response with a null body status. It may
         // change in the future.
-        if (nullBodyStatus.includes(init.status) && body === "") body = null;
+        if (nullBodyStatus.includes(init.status)) body = null;
       }
       super(new BaseResponse(body, init));
     }

@lukaszczerpak
Copy link
Author

if you are happy with the proposal I can create a PR?

@mrbbot
Copy link
Contributor

mrbbot commented Feb 7, 2022

Hey! 👋 I've just released version 2.3.0 including a fix for this. You can find the full changelog here.

@mrbbot mrbbot closed this as completed Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behaviour mismatch Different behaviour to Workers runtime
Projects
None yet
Development

No branches or pull requests

2 participants