Deno: Fetch leaks resources if body is not resolved.

[olaven]

Hi 👋
I believe I have found an issue where the fetch-api leaks resources if the
body is not resolved, e.g. with .text()/.json().

Given the following test:

import { assertEquals } from "https://deno.land/std/testing/asserts.ts";

Deno.test("fetching TODOs", async () => {

        const response = await fetch("https://jsonplaceholder.typicode.com/todos/");
        //await response.json(); //NOTE: adding this line removes the issue (or .text()/similar)
        assertEquals(200, response.status);
});

Produces the following output:

running 1 tests
test fetching TODOs ... FAILED (94ms)

failures:

fetching TODOs
Error: Test case is leaking resources.
Before: {
  "0": "stdin",
  "1": "stdout",
  "2": "stderr"
}
After: {
  "0": "stdin",
  "1": "stdout",
  "2": "stderr",
  "3": "httpBody"
}
    at Object.assert ($deno$/util.ts:25:11)
    at Object.resourceSanitizer [as fn] ($deno$/testing.ts:75:5)
    at async TestApi.[Symbol.asyncIterator] ($deno$/testing.ts:259:11)
    at async Object.runTests ($deno$/testing.ts:341:20)

failures:

    fetching TODOs

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out (95ms)

The issue is only present from v0.37.0.

[syumai]

@olaven You should close response body.
Deno's fetch returns a Response with Body which implements Deno.ReadCloser.
text() and json() are calling Body.#bodyBuffer() which closes body implicitly.
Here's an example to resolve this issue.

import { assertEquals } from "https://deno.land/std/testing/asserts.ts";

Deno.test("fetching TODOs", async () => {
  const response = await fetch("https://jsonplaceholder.typicode.com/todos/");
  await response.body.close();
  assertEquals(200, response.status);
});

[olaven]

Thanks! I don't believe I have come across this before 🤔
Does this apply to browser implementations as well?
Is this behaviour intended?

[syumai]

It's not in browser's behavior. It's Deno's behavior (not in fetch standard).
Deno's fetch is more controllable than browser's fetch.
We can controll response body's resource explicitly.
Maybe Deno refers Go's HTTP implementation like this:

resp, _ := http.Get("http://example.com/")
defer resp.Body.Close() // Body must be closed explicitly

However, I also think this could confuse JS land's programmers :sweat_smile:

[lucacasonato]

Well its a side effect of how fetch is implemented in Deno, but I think its a bug nonetheless because our fetch implementation should follow the spec, and this is not part of the spec.

[ry]

I've recently removed type declarations for response.body.close() as it's non-standard. I'm about to undertake a rewriting of fetch which will address these API issues. That said, even after this refactor, you will still need to consume the body in tests. Our tests check that resources are cleaned up. I suggest adding something like

await response.arrayBuffer();

to fix the above test failure.

[alisabzevari]

As a followup question to this issue, wouldn't it be beneficial to close the resource after iterating over the body?

In other words, the following code fails as well:

Deno.test({
  name: "test",
  fn: async () => {
    const response = await fetch("https://api.github.com/users/http4ts/repos");

    await readToEnd(response.body!);
  }
});

async function readToEnd(it: AsyncIterable<Uint8Array>) {
  let chunkCount = 0;
  for await (const chunk of it) {
    chunkCount += chunk.length;
  }
  console.log(`Finished reading ${chunkCount} bytes.`);
}