auto-close httpConn on error or connection completed

denoland · Apr 8, 2021 · 6c74a71 · 6c74a71
1 parent 5a081cb
commit 6c74a71
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 31 deletions.
diff --git a/cli/dts/lib.deno.unstable.d.ts b/cli/dts/lib.deno.unstable.d.ts
@@ -1212,10 +1212,13 @@ declare namespace Deno {
    * Parse HTTP requests from the given connection
    *
    * ```ts
-   * const requests = await Deno.startHttp(conn);
+   * const httpConn = await Deno.startHttp(conn);
+   * const { request, respondWith } = await httpConn.next();
+   * respondWith(new Response("Hello World"));
    * ```
    *
-   * It is the caller's responsibility to call close on the HttpConn.
+   * If `httpConn.next()` encounters an error or returns `done == true` then
+   * the underlying HttpConn resource is closed automatically.
    */
   export function startHttp(conn: Conn): HttpConn;
 }

diff --git a/cli/tests/unit/http_test.ts b/cli/tests/unit/http_test.ts
@@ -12,12 +12,11 @@ unitTest({ perms: { net: true } }, async function httpServerBasic() {
   const promise = (async () => {
     const listener = Deno.listen({ port: 4501 });
     for await (const conn of listener) {
-      const requests = Deno.startHttp(conn);
-      for await (const { request, respondWith } of requests) {
+      const httpConn = Deno.startHttp(conn);
+      for await (const { request, respondWith } of httpConn) {
         assertEquals(await request.text(), "");
         respondWith(new Response("Hello World"));
       }
-      requests.close();
       break;
     }
   })();
@@ -42,12 +41,12 @@ unitTest(
     const promise = (async () => {
       const listener = Deno.listen({ port: 4501 });
       const conn = await listener.accept();
-      const requests = Deno.startHttp(conn);
-      const { value: { request, respondWith }, done } = await requests.next();
+      const httpConn = Deno.startHttp(conn);
+      const { value: { request, respondWith }, done } = await httpConn.next();
       assert(!done);
       assert(!request.body);
       await respondWith(new Response(stream.readable));
-      requests.close();
+      httpConn.close();
       listener.close();
     })();
 
@@ -70,19 +69,18 @@ unitTest(
     const promise = (async () => {
       const listener = Deno.listen({ port: 4501 });
       const conn = await listener.accept();
-      const requests = Deno.startHttp(conn);
-      const { value: { request, respondWith } } = await requests.next();
+      const httpConn = Deno.startHttp(conn);
+      const { value: { request, respondWith } } = await httpConn.next();
       const reqBody = await request.text();
       assertEquals("hello world", reqBody);
       await respondWith(new Response(""));
 
-      // TODO(ry) If we don't call requests.next() here we get "error sending
+      // TODO(ry) If we don't call httpConn.next() here we get "error sending
       // request for url (https://localhost:4501/): connection closed before
       // message completed".
-      const { done } = await requests.next();
+      const { done } = await httpConn.next();
       assert(done);
 
-      requests.close();
       listener.close();
     })();
 
@@ -101,11 +99,11 @@ unitTest({ perms: { net: true } }, async function httpServerStreamDuplex() {
   const promise = (async () => {
     const listener = Deno.listen({ port: 4501 });
     const conn = await listener.accept();
-    const requests = Deno.startHttp(conn);
-    const { value: { request, respondWith } } = await requests.next();
+    const httpConn = Deno.startHttp(conn);
+    const { value: { request, respondWith } } = await httpConn.next();
     assert(request.body);
     await respondWith(new Response(request.body));
-    requests.close();
+    httpConn.close();
     listener.close();
   })();
 
@@ -134,28 +132,28 @@ unitTest({ perms: { net: true } }, async function httpServerStreamDuplex() {
 unitTest({ perms: { net: true } }, async function httpServerClose() {
   const listener = Deno.listen({ port: 4501 });
   const client = await Deno.connect({ port: 4501 });
-  const requests = Deno.startHttp(await listener.accept());
+  const httpConn = Deno.startHttp(await listener.accept());
   client.close();
-  const { done } = await requests.next();
+  const { done } = await httpConn.next();
   assert(done);
-  requests.close();
+  // Note httpConn is automatically closed when "done" is reached.
   listener.close();
 });
 
 unitTest({ perms: { net: true } }, async function httpServerInvalidMethod() {
   const listener = Deno.listen({ port: 4501 });
   const client = await Deno.connect({ port: 4501 });
-  const requests = Deno.startHttp(await listener.accept());
+  const httpConn = Deno.startHttp(await listener.accept());
   await client.write(new Uint8Array([1, 2, 3]));
   await assertThrowsAsync(
     async () => {
-      await requests.next();
+      await httpConn.next();
     },
     Deno.errors.Http,
     "invalid HTTP method parsed",
   );
+  // Note httpConn is automatically closed when it errors.
   client.close();
-  requests.close();
   listener.close();
 });
 
@@ -173,17 +171,16 @@ unitTest(
         keyFile: "cli/tests/tls/localhost.key",
       });
       const conn = await listener.accept();
-      const requests = Deno.startHttp(conn);
-      const { value: { request, respondWith } } = await requests.next();
+      const httpConn = Deno.startHttp(conn);
+      const { value: { request, respondWith } } = await httpConn.next();
       await respondWith(new Response("Hello World"));
 
-      // TODO(ry) If we don't call requests.next() here we get "error sending
+      // TODO(ry) If we don't call httpConn.next() here we get "error sending
       // request for url (https://localhost:4501/): connection closed before
       // message completed".
-      const { done } = await requests.next();
+      const { done } = await httpConn.next();
       assert(done);
 
-      requests.close();
       listener.close();
     })();
 

diff --git a/runtime/js/40_http.js b/runtime/js/40_http.js
@@ -51,7 +51,6 @@
 
         if (connectionClosed) {
           if (responseSenderRid === 0) {
-            //core.close(this.#rid);
             return { done: true };
           } else {
             throw Error("unhandled");

diff --git a/runtime/ops/http.rs b/runtime/ops/http.rs
@@ -151,11 +151,24 @@ async fn op_http_request_next(
   poll_fn(|cx| {
     let connection_closed = match conn_resource.poll(cx) {
       Poll::Pending => false,
-      Poll::Ready(Ok(())) => true,
+      Poll::Ready(Ok(())) => {
+        // close ConnResource
+        state
+          .borrow_mut()
+          .resource_table
+          .take::<ConnResource>(conn_rid)
+          .unwrap();
+        true
+      }
       Poll::Ready(Err(e)) => {
         // close ConnResource
-        // close RequestResource associated with connection
-        // close ResponseBodyResource associated with connection
+        state
+          .borrow_mut()
+          .resource_table
+          .take::<ConnResource>(conn_rid)
+          .unwrap();
+        // TODO(ry) close RequestResource associated with connection
+        // TODO(ry) close ResponseBodyResource associated with connection
         return Poll::Ready(Err(e));
       }
     };