Document requirement to handle `error.response` when catching `HTTPError`

[daweijs]

Reproduction with deno :

ky.deno.test.ts :

import ky from "https://esm.sh/[email protected]";

Deno.test("throws HTTPError : no memory leak", async () => {
  // this test throws the expected error and does not leak memory
  // HTTPError: Request failed with status code 404 Not Found: GET https://github.com/not_found

  await ky.get("https://github.com/not_found").text();
});

Deno.test("memory leak when catching error", async () => {
  // this test report a memory leak when catching the error
  // the memory leak is fixed if we rethrow something

  // the test fails with :
  // Leaks detected:
  // - A fetch response body was created during the test, but not consumed during the test. Consume or close the response body `ReadableStream`, e.g `await resp.text()` or `await resp.body.cancel()`.

  try {
    await ky.get("https://github.com/not_found").text();
  } catch (error) {
    console.error("I handle the error here");

    // fix the leak : rethrow the error
    // throw error;
  }
});

deno test --allow-net ky.deno.test.ts

Same issue with bun : https://github.com/denoland/deno/issues/25546#issuecomment-2341318464

[sholladay]

Nice test cases, thanks!

This happens because when an HTTPError is thrown, it has error.response with a body stream whose buffer needs be either consumed or discarded. Currently, Ky doesn't do either internally and so it is up to the user to do so. That's not ideal, nor is it obvious, but I'm not sure there's an obviously better approach at the moment. When we throw the HTTPError, we could wrap it in a try / finally that cancels the body stream. But that means you can't do anything with it after your catch block, which is equally surprising.

[sholladay]

There are actually two tasks here:

1. The docs need to be updated to explain that the error response stream must either be consumed or discarded if the error is caught. Currently, the docs only show how to consume it and don't even mention discarding it.
2. We can consider a breaking change to automatically consume or discard the stream somehow, perhaps by calling error.response.arrayBuffer() internally, But that takes control away from the user and prevents you from being able to access the raw stream, which you might want to. I'm hoping that the using proposal will come in handy here, allowing us to discard the stream when it goes out of scope.

[sholladay]

All of the major browsers and server runtimes now support using, except for Safari. The technical preview for Safari has it, too. So should be any day now when we can give this a try.