From c3c69aff7e1df1a9480d2a5e9a0fa17cf3af6409 Mon Sep 17 00:00:00 2001
From: "Kevin (Kun) \"Kassimo\" Qian" <kevinkassimo@gmail.com>
Date: Wed, 11 Dec 2019 16:46:03 -0800
Subject: [PATCH] fix(std/http): close connection on .respond() error (#3475)

---
 std/http/server.ts      | 29 +++++++++++++++---
 std/http/server_test.ts | 67 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 90 insertions(+), 6 deletions(-)

diff --git a/std/http/server.ts b/std/http/server.ts
index ce32511b1a..71c145d6f4 100644
--- a/std/http/server.ts
+++ b/std/http/server.ts
@@ -107,7 +107,7 @@ export class ServerRequest {
   conn!: Conn;
   r!: BufReader;
   w!: BufWriter;
-  done: Deferred<void> = deferred();
+  done: Deferred<Error | undefined> = deferred();
 
   public async *bodyStream(): AsyncIterableIterator<Uint8Array> {
     if (this.headers.has("content-length")) {
@@ -193,11 +193,24 @@ export class ServerRequest {
   }
 
   async respond(r: Response): Promise<void> {
-    // Write our response!
-    await writeResponse(this.w, r);
+    let err: Error | undefined;
+    try {
+      // Write our response!
+      await writeResponse(this.w, r);
+    } catch (e) {
+      try {
+        // Eagerly close on error.
+        this.conn.close();
+      } catch {}
+      err = e;
+    }
     // Signal that this request has been processed and the next pipelined
     // request on the same connection can be accepted.
-    this.done.resolve();
+    this.done.resolve(err);
+    if (err) {
+      // Error during responding, rethrow.
+      throw err;
+    }
   }
 }
 
@@ -338,7 +351,13 @@ export class Server implements AsyncIterable<ServerRequest> {
 
       // Wait for the request to be processed before we accept a new request on
       // this connection.
-      await req!.done;
+      const procError = await req!.done;
+      if (procError) {
+        // Something bad happened during response.
+        // (likely other side closed during pipelined req)
+        // req.done implies this connection already closed, so we can just return.
+        return;
+      }
     }
 
     if (req! === Deno.EOF) {
diff --git a/std/http/server_test.ts b/std/http/server_test.ts
index 30070b272d..557223164e 100644
--- a/std/http/server_test.ts
+++ b/std/http/server_test.ts
@@ -17,7 +17,7 @@ import {
   readRequest,
   parseHTTPVersion
 } from "./server.ts";
-import { delay } from "../util/async.ts";
+import { delay, deferred } from "../util/async.ts";
 import {
   BufReader,
   BufWriter,
@@ -590,4 +590,69 @@ test({
   }
 });
 
+// TODO(kevinkassimo): create a test that works on Windows.
+// The following test is to ensure that if an error occurs during respond
+// would result in connection closed. (such that fd/resource is freed).
+// On *nix, a delayed second attempt to write to a CLOSE_WAIT connection would
+// receive a RST and thus trigger an error during response for us to test.
+// We need to find a way to similarly trigger an error on Windows so that
+// we can test if connection is closed.
+if (Deno.build.os !== "win") {
+  test({
+    name: "[http] respond error handling",
+    async fn(): Promise<void> {
+      const connClosedPromise = deferred();
+      const serverRoutine = async (): Promise<void> => {
+        let reqCount = 0;
+        const server = serve(":8124");
+        const serverRid = server.listener["rid"];
+        let connRid = -1;
+        for await (const req of server) {
+          connRid = req.conn.rid;
+          reqCount++;
+          await req.body();
+          await connClosedPromise;
+          try {
+            await req.respond({
+              body: new TextEncoder().encode("Hello World")
+            });
+            await delay(100);
+            req.done = deferred();
+            // This duplicate respond is to ensure we get a write failure from the
+            // other side. Our client would enter CLOSE_WAIT stage after close(),
+            // meaning first server .send (.respond) after close would still work.
+            // However, a second send would fail under RST, which is similar
+            // to the scenario where a failure happens during .respond
+            await req.respond({
+              body: new TextEncoder().encode("Hello World")
+            });
+          } catch {
+            break;
+          }
+        }
+        server.close();
+        const resources = Deno.resources();
+        assert(reqCount === 1);
+        // Server should be gone
+        assert(!(serverRid in resources));
+        // The connection should be destroyed
+        assert(!(connRid in resources));
+      };
+      const p = serverRoutine();
+      const conn = await Deno.dial({
+        hostname: "127.0.0.1",
+        port: 8124
+      });
+      await Deno.writeAll(
+        conn,
+        new TextEncoder().encode("GET / HTTP/1.1\r\n\r\n")
+      );
+      conn.close(); // abruptly closing connection before response.
+      // conn on server side enters CLOSE_WAIT state.
+      connClosedPromise.resolve();
+      await p;
+    }
+  });
+}
+
 runIfMain(import.meta);