search

denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
93.98k stars 5.23k forks source link

http/server: ERR_CONNECTION_RESET when responding without request body consumed #21209

Open jespertheend opened 2 years ago

jespertheend commented 2 years ago

Describe the bug

Steps to Reproduce

  1. Create main.ts:
async function post() {
    const fileInput = document.getElementById("fileInput") as HTMLInputElement;
    if (!fileInput.files || fileInput.files.length != 1) {
        alert("Please select a file");
        return;
    }
    const file = fileInput.files[0];
    const response = await fetch("/post", {
        method: "POST",
        headers: {
            "content-type": "application/zip"
        },
        body: file,
    });
    alert(await response.text());
}

Deno.serve(async (req: Request) => {
    const url = new URL(req.url);
    if (url.pathname == "/") {
        return new Response(`
<!DOCTYPE html>
<html>
    <body>
        <script>
            ${post.toString()}
        </script>
        <input type="file" id="fileInput" />
        <button onclick="post()">POST</button>
    </body>
</html>`, {
            headers: {
                "content-type": "text/html"
            }
        })
    } else if (url.pathname == "/post") {
        console.log("/post received");
        // Uncomment this to fix the issue:
        // const buffer = await req.arrayBuffer();
        return new Response("OK");
    }
    return new Response("Not found", {
        status: 404,
    })
})
  1. Run deno run --allow-net main.ts
  2. Visit http://localhost:8000/
  3. Select a large file (in my case I'm using dist.zip)
  4. Open the network panel in the browser inspector
  5. Click 'POST'.

Expected behavior

The request responds with 'ok'

Actual behavior

The request fails with ERR_CONNECTION_RESET. Note that the deno console doesn't show any errors and the console.log("/post received"); line is being run. If you uncomment the await req.arrayBuffer() line, the issue is fixed.

Environment

iuioiua commented 2 years ago

Hi @jespertheend, this is likely because the req.body needs to be consumed. Otherwise, your handler is leaking resources. It's best practise to always consume your req.body, even if it doesn't need to be used, per se. For example:

async handler(req: Request): Promise<Response> {
  // Consumes `req.body` without really using it.
  await req.body?.cancel();
  return new Response("OK"):
}
jespertheend commented 2 years ago

Hmm ok that makes sense. I could swear I ran into issues earlier where the same ERR_CONNECTION_RESET would happen even when returning 404 responses or throwing errors, but I can't seem to reproduce this anymore. So this is arguably less of a problem than I initially thought.

iuioiua commented 2 years ago

I’ll test this out on my machine and see how I go. Also, the dist.zip you shared is only 600kb. However, you say it’s large. Is this definitely the same file you tested with?

jespertheend commented 2 years ago

Yes, I'm assuming this happens once a file reaches a certain size, because I tried it with a smaller file of a couple kb and I didn't see this issue. I'm also now running into the issue on an ubuntu server even though I'm returning a 404 response, and doing the same thing on my local machine doesn't give this issue. So sounds like it's running into some sort of race condition.

jespertheend commented 2 years ago

Seems like using serveTls rather than serve has an effect as well. I've run into a case where switching to serveTls causes an ERR_CONNECTION_RESET whereas using serve does not.

iuioiua commented 1 year ago

Hi @jespertheend, are you still encountering this issue? If so, what's the minimal reproducible snippet?

jespertheend commented 1 year ago

Yes I can still reproduce with the snippet from the first comment. I can reproduce it on both macOS and Ubuntu. It seems a bit flaky though, about 1 in ten times it works fine, but the other 90% it fails with ERR_CONNECTION_RESET

iuioiua commented 1 year ago

Frankly, I don't think this is an issue with the server. Instead, I think the code snippet is flawed due to its hackiness. If you're trying to simply make a POST request, you can do so with just HTML. A tutorial on how to do so can be found here.

jespertheend commented 1 year ago

That example is missing method="post" and enctype="multipart/form-data" and as a result no file is uploaded. Only the file name is provided in the query string.

If you wish to achieve this using only html, you can use this reduced test case:

import { serve } from "https://deno.land/std@0.165.0/http/server.ts";

serve(async (req: Request) => {
    const url = new URL(req.url);
    if (url.pathname == "/") {
        return new Response(
            `
<!DOCTYPE html>
<html>
    <body>
        <form action="/post" method="post" enctype="multipart/form-data">
            <input type="file" id="file" name="file">
            <input type="submit">
        </form>
    </body>
</html>`,
            {
                headers: {
                    "content-type": "text/html",
                },
            },
        );
    } else if (url.pathname == "/post") {
        console.log("/post received");
        // Uncomment this to fix the issue:
        // const buffer = await req.arrayBuffer();
        return new Response("OK");
    }
    return new Response("Not found", {
        status: 404,
    });
});

In this case the issue is arguably worse, because now the user gets redirected to an empty page with a full screen error, rather than only an error being printed in the console.

iuioiua commented 10 months ago

Hi @jespertheend, it's been a year since the last update. Is this still an issue for you?

jespertheend commented 10 months ago

I can still reproduce this but with Deno.serve() instead of using std. So I guess this issue should be moved to the deno repository.

New code snippet

```js async function post() { const fileInput = document.getElementById("fileInput") as HTMLInputElement; if (!fileInput.files || fileInput.files.length != 1) { alert("Please select a file"); return; } const file = fileInput.files[0]; const response = await fetch("/post", { method: "POST", headers: { "content-type": "application/zip" }, body: file, }); alert(await response.text()); } Deno.serve(async (req: Request) => { const url = new URL(req.url); if (url.pathname == "/") { return new Response(` `, { headers: { "content-type": "text/html" } }) } else if (url.pathname == "/post") { console.log("/post received"); // Uncomment this to fix the issue: // const buffer = await req.arrayBuffer(); return new Response("OK"); } return new Response("Not found", { status: 404, }) }) ```

But it's not a big issue for me so I'd be fine with closing it as well.

iuioiua commented 10 months ago

@bartlomieju, are you able to transfer this to the runtime repo?

kt3k commented 10 months ago

curl client also shows the error like the below (though the response itself looks returned correctly):

$ curl -X POST localhost:8000/post -d @$HOME/Downloads/dist.zip
curl: (55) Send failure: Broken pipe
OK

I updated the original example to the updated one.