-
Notifications
You must be signed in to change notification settings - Fork 331
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
workerd throws "Can't read from request stream after response has been sent" after responding to request with unconsumed body #1730
Comments
You should only be seeing this exception if you actually try to use the Request object after the response has been fully sent. If you don't touch the object, no error should be seen. I tried writing a simple worker that doesn't read the request body, then sent it a POST request with a body. As expected, I did not observe this error being logged. When I added a Can you please provide self-contained sample code which reproduces the error, where you believe the error is spurious? |
Hi Kenton, here's a minimal reproduction:
{
"dependencies": {
"vitest": "^1.3.1",
"wrangler": "^3.29.0"
}
}
export default {
async fetch() {
return new Response("ok");
},
};
import { expect, test } from "vitest";
import { unstable_dev } from "wrangler";
const dev = await unstable_dev("./index.js");
test("this always passes", async () => {
const res = await dev.fetch("/", { method: "POST", body: "body" });
expect(res.status).toBe(200);
});
test("these will randomly fail", async () => {
const res = await dev.fetch("/", { method: "POST", body: "body" });
expect(res.status).toBe(200);
});
test("these will randomly fail", async () => {
const res = await dev.fetch("/", { method: "POST", body: "body" });
expect(res.status).toBe(200);
});
test("these will randomly fail", async () => {
const res = await dev.fetch("/", { method: "POST", body: "body" });
expect(res.status).toBe(200);
});
test("these will randomly fail", async () => {
const res = await dev.fetch("/", { method: "POST", body: "body" });
expect(res.status).toBe(200);
});
test("these will randomly fail", async () => {
const res = await dev.fetch("/", { method: "POST", body: "body" });
expect(res.status).toBe(200);
});
test("these will randomly fail", async () => {
const res = await dev.fetch("/", { method: "POST", body: "body" });
expect(res.status).toBe(200);
}); Then run the tests sequentially: vitest run --pool forks --poolOptions.forks.singleFork |
Oh, I wonder if the error is actually coming from the proxy that workers-sdk sets up during testing, which also runs on workerd? So it's not actually your worker complaining, it's the test infrastructure. @mrbbot is that possible? |
(If that's the case, then this is not a workerd bug, it's a bug in the workers-sdk proxy.) |
@kentonv Potentially. The proxy |
@mrbbot the assertion fails with res.status 500, and the response body is "Error: Network connection lost" Turning on debug logging from unstable_dev, I can see two different errors being logged:
|
Ah ok. Given the |
Hi @mrbbot I just tested the repro against 3.30.1 and can confirm the issue is resolved. Thanks for your help! |
This issue still exists, and I do not see a solution. I still have this issue in [email protected] |
@AbakirH it's fixed for that test-case, at least. Since it looks like the issue was in wrangler (Proxy Durable Object) maybe you could open a new issue in that repo with a reproduction? |
This issue still persists. |
This might be a duplicate of #918 but opening as a seperate issue for visibility (and incase the other issue is indeed related to DurableObjects only).
We were experiencing consistent failures of certain tests in our API test suite (using ustable_dev and vite). After investigation, it seems the failures were actually caused by a workerd exception from the test before each failing test. This only became clear after running the tests in sequence and turning on debug level logging for unstable_dev.
It looks like workerd throws this exception (originating here) any time a request body is not consumed and a response is sent. So, in our case, I was checking authz first in handlers and throwing on fail, and then parsing request body. Parsing the body first fixed the failures.
However, clients passing request bodies where one isn't expected should in theory cause the same exception in production. I guess this is fine because Cloudflare just spins up another worker and gets on with it's life, but this seems like it should be avoided.
The text was updated successfully, but these errors were encountered: