fix: request wasn't timing out waiting for body (#227)

src/request.test.ts

@@ -34,6 +34,51 @@ function withServer(action: (serverUrl: URL) => Promise<void>) {
       } else if (url.pathname.startsWith("/code/")) {
         const code = parseInt(url.pathname.replace(/^\/code\//, ""), 0);
         return new Response(code.toString(), { status: code });
+      } else if (url.pathname.startsWith("/sleep/")) {
+        const ms = parseInt(url.pathname.replace(/^\/sleep\//, ""), 0);
+        const signal = request.signal;
+        return new Promise((resolve, reject) => {
+          const timeoutId = setTimeout(() => {
+            resolve(new Response("", { status: 200 }));
+          }, ms);
+          signal.addEventListener("abort", () => {
+            clearTimeout(timeoutId);
+            reject(new Error("Client aborted."));
+          });
+        });
+      } else if (url.pathname.startsWith("/sleep-body/")) {
+        const ms = parseInt(url.pathname.replace(/^\/sleep-body\//, ""), 0);
+        const signal = request.signal;
+        const abortController = new AbortController();
+        return new Response(
+          new ReadableStream({
+            start(controller) {
+              return new Promise((resolve, reject) => {
+                if (signal.aborted || abortController.signal.aborted) {
+                  return;
+                }
+                const timeoutId = setTimeout(() => {
+                  controller.close();
+                  resolve();
+                }, ms);
+                signal.addEventListener("abort", () => {
+                  clearTimeout(timeoutId);
+                  reject(new Error("Client aborted."));
+                });
+                abortController.signal.addEventListener("abort", () => {
+                  clearTimeout(timeoutId);
+                  reject(new Error("Client aborted."));
+                });
+              });
+            },
+            cancel(reason) {
+              abortController.abort(reason);
+            },
+          }),
+          {
+            status: 200,
+          },
+        );
       } else {
         return new Response("Not Found", { status: 404 });
       }
@@ -234,6 +279,21 @@ Deno.test("$.request", (t) => {
       );
     });
 
+    step("ensure times out  waiting for body", async () => {
+      const request = new RequestBuilder()
+        .url(new URL("/sleep-body/10000", serverUrl))
+        .timeout(50)
+        .showProgress();
+      const response = await request.fetch();
+      let caughtErr: unknown;
+      try {
+        await response.text();
+      } catch (err) {
+        caughtErr = err;
+      }
+      assertEquals(caughtErr, "Request timed out after 50 milliseconds.");
+    });
+
     await Promise.all(steps);
   });
 });

src/request.ts

@@ -1,3 +1,4 @@
+import { formatMillis } from "./common.ts";
 import { Delay, delayToMs, filterEmptyRecordValues, getFileNameFromUrl } from "./common.ts";
 import { ProgressBar } from "./console/mod.ts";
 import { PathRef } from "./path.ts";
@@ -336,20 +337,31 @@ export class RequestBuilder implements PromiseLike<RequestResult> {
   }
 }
 
+interface Timeout {
+  signal: AbortSignal;
+  clear(): void;
+}
+
 /** Result of making a request. */
 export class RequestResult {
   #response: Response;
   #downloadResponse: Response;
   #originalUrl: string;
+  #timeout: Timeout | undefined;
 
   /** @internal */
   constructor(opts: {
     response: Response;
     originalUrl: string;
     progressBar: ProgressBar | undefined;
+    timeout: Timeout | undefined;
   }) {
     this.#originalUrl = opts.originalUrl;
     this.#response = opts.response;
+    this.#timeout = opts.timeout;
+    if (opts.response.body == null) {
+      this.#timeout?.clear();
+    }
 
     if (opts.progressBar != null) {
       const pb = opts.progressBar;
@@ -363,11 +375,17 @@ export class RequestResult {
             try {
               while (true) {
                 const { done, value } = await reader.read();
-                if (done || value == null) break;
+                if (done || value == null) {
+                  break;
+                }
                 pb.increment(value.byteLength);
                 controller.enqueue(value);
               }
-              controller.close();
+              if (opts.timeout?.signal?.aborted) {
+                controller.error(opts.timeout.signal.reason);
+              } else {
+                controller.close();
+              }
             } finally {
               reader.releaseLock();
               pb.finish();
@@ -400,6 +418,11 @@ export class RequestResult {
     return this.#response.redirected;
   }
 
+  /** The underlying `AbortSignal` if a delay or signal was specified. */
+  get signal(): AbortSignal | undefined {
+    return this.#timeout?.signal;
+  }
+
   /** Status code of the response. */
   get status(): number {
     return this.#response.status;
@@ -436,11 +459,15 @@ export class RequestResult {
    * Note: Returns `undefined` when `.noThrow(404)` and status code is 404.
    */
   async arrayBuffer(): Promise<ArrayBuffer> {
-    if (this.#response.status === 404) {
-      await this.#response.body?.cancel();
-      return undefined!;
+    try {
+      if (this.#response.status === 404) {
+        await this.#response.body?.cancel();
+        return undefined!;
+      }
+      return this.#downloadResponse.arrayBuffer();
+    } finally {
+      this.#timeout?.clear();
     }
-    return this.#downloadResponse.arrayBuffer();
   }
 
   /**
@@ -449,11 +476,15 @@ export class RequestResult {
    * Note: Returns `undefined` when `.noThrow(404)` and status code is 404.
    */
   async blob(): Promise<Blob> {
-    if (this.#response.status === 404) {
-      await this.#response.body?.cancel();
-      return undefined!;
+    try {
+      if (this.#response.status === 404) {
+        await this.#response.body?.cancel();
+        return undefined!;
+      }
+      return await this.#downloadResponse.blob();
+    } finally {
+      this.#timeout?.clear();
     }
-    return this.#downloadResponse.blob();
   }
 
   /**
@@ -462,11 +493,15 @@ export class RequestResult {
    * Note: Returns `undefined` when `.noThrow(404)` and status code is 404.
    */
   async formData(): Promise<FormData> {
-    if (this.#response.status === 404) {
-      await this.#response.body?.cancel();
-      return undefined!;
+    try {
+      if (this.#response.status === 404) {
+        await this.#response.body?.cancel();
+        return undefined!;
+      }
+      return await this.#downloadResponse.formData();
+    } finally {
+      this.#timeout?.clear();
     }
-    return this.#downloadResponse.formData();
   }
 
   /**
@@ -475,11 +510,15 @@ export class RequestResult {
    * Note: Returns `undefined` when `.noThrow(404)` and status code is 404.
    */
   async json<TResult = any>(): Promise<TResult> {
-    if (this.#response.status === 404) {
-      await this.#response.body?.cancel();
-      return undefined as any;
+    try {
+      if (this.#response.status === 404) {
+        await this.#response.body?.cancel();
+        return undefined as any;
+      }
+      return await this.#downloadResponse.json();
+    } finally {
+      this.#timeout?.clear();
     }
-    return this.#downloadResponse.json();
   }
 
   /**
@@ -488,19 +527,27 @@ export class RequestResult {
    * Note: Returns `undefined` when `.noThrow(404)` and status code is 404.
    */
   async text(): Promise<string> {
-    if (this.#response.status === 404) {
-      // most people don't need to bother with this and if they do, they will
-      // need to opt-in with `noThrow()`. So just assert non-nullable
-      // to make it easier to work with and highlight this behaviour in the jsdocs.
-      await this.#response.body?.cancel();
-      return undefined!;
+    try {
+      if (this.#response.status === 404) {
+        // most people don't need to bother with this and if they do, they will
+        // need to opt-in with `noThrow()`. So just assert non-nullable
+        // to make it easier to work with and highlight this behaviour in the jsdocs.
+        await this.#response.body?.cancel();
+        return undefined!;
+      }
+      return await this.#downloadResponse.text();
+    } finally {
+      this.#timeout?.clear();
     }
-    return this.#downloadResponse.text();
   }
 
   /** Pipes the response body to the provided writable stream. */
-  pipeTo(dest: WritableStream<Uint8Array>, options?: PipeOptions): Promise<void> {
-    return this.readable.pipeTo(dest, options);
+  async pipeTo(dest: WritableStream<Uint8Array>, options?: PipeOptions): Promise<void> {
+    try {
+      await this.readable.pipeTo(dest, options);
+    } finally {
+      this.#timeout?.clear();
+    }
   }
 
   /**
@@ -555,6 +602,7 @@ export class RequestResult {
         } catch {
           // do nothing
         }
+        this.#timeout?.clear();
       }
     } catch (err) {
       await this.#response.body?.cancel();
@@ -599,11 +647,11 @@ export async function makeRequest(state: RequestBuilderState) {
     referrerPolicy: state.referrerPolicy,
     signal: timeout?.signal,
   });
-  timeout?.clear();
   const result = new RequestResult({
     response,
     originalUrl: state.url.toString(),
     progressBar: getProgressBar(),
+    timeout,
   });
   if (!state.noThrow) {
     result.throwIfNotOk();
@@ -633,12 +681,16 @@ export async function makeRequest(state: RequestBuilderState) {
     }
   }
 
-  function getTimeout() {
+  function getTimeout(): Timeout | undefined {
     if (state.timeout == null) {
       return undefined;
     }
+    const timeout = state.timeout;
     const controller = new AbortController();
-    const timeoutId = setTimeout(() => controller.abort(), state.timeout);
+    const timeoutId = setTimeout(
+      () => controller.abort(`Request timed out after ${formatMillis(timeout)}.`),
+      timeout,
+    );
     return {
       signal: controller.signal,
       clear() {