http: don't destroy completed request

Calling destroy() on a completed ClientRequest, i.e. once 'close' will be emitted should be a noop. Also before emitting 'close' destroyed === true. Fixes: #32851
nodejs · ronag · Apr 28, 2020 · May 9, 2020 · May 9, 2020 · 8ff38ec989e6c7adeb17c8ecbf8d0bd1232fae21
commit 8ff38ec989e6c7adeb17c8ecbf8d0bd1232fae21
diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -413,6 +413,8 @@ function socketCloseListener() {
   // the `socketOnData`.
   const parser = socket.parser;
   const res = req.res;
+
+  req.destroyed = true;
   if (res) {
     // Socket closed before we emitted 'end' below.
     if (!res.complete) {
@@ -667,7 +669,9 @@ function responseKeepAlive(req) {
   // handlers have a chance to run.
   defaultTriggerAsyncIdScope(asyncId, process.nextTick, emitFreeNT, req);
 
+  req.destroyed = true;
   if (req.res) {
+    // TODO(ronag): res.destroyed=true?
     // Detach socket from IncomingMessage to avoid destroying the freed
     // socket in IncomingMessage.destroy().
     req.res.socket = null;

diff --git a/test/parallel/test-http-client-agent-abort-close-event.js b/test/parallel/test-http-client-agent-abort-close-event.js
@@ -1,5 +1,6 @@
 'use strict';
 const common = require('../common');
+const assert = require('assert');
 const http = require('http');
 
 const server = http.createServer(common.mustNotCall());
@@ -16,6 +17,7 @@ server.listen(0, common.mustCall(() => {
     .on('socket', common.mustNotCall())
     .on('response', common.mustNotCall())
     .on('close', common.mustCall(() => {
+      assert.strictEqual(req.destroyed, true);
       server.close();
       keepAliveAgent.destroy();
     }))

diff --git a/test/parallel/test-http-client-agent-end-close-event.js b/test/parallel/test-http-client-agent-end-close-event.js
@@ -1,5 +1,6 @@
 'use strict';
 const common = require('../common');
+const assert = require('assert');
 const http = require('http');
 
 const server = http.createServer(common.mustCall((req, res) => {
@@ -18,6 +19,7 @@ server.listen(0, common.mustCall(() => {
     .on('response', common.mustCall((res) => {
       res
         .on('close', common.mustCall(() => {
+          assert.strictEqual(req.destroyed, true);
           server.close();
           keepAliveAgent.destroy();
         }))

diff --git a/test/parallel/test-http-client-close-event.js b/test/parallel/test-http-client-close-event.js
@@ -22,6 +22,7 @@ server.listen(0, common.mustCall(() => {
   }));
 
   req.on('close', common.mustCall(() => {
+    assert.strictEqual(req.destroyed, true);
     assert.strictEqual(errorEmitted, true);
     server.close();
   }));

diff --git a/test/parallel/test-http-client-override-global-agent.js b/test/parallel/test-http-client-override-global-agent.js
@@ -21,6 +21,8 @@ function makeRequest() {
   const req = http.get({
     port: server.address().port
   });
-  req.on('close', () =>
-    server.close());
+  req.on('close', () => {
+    assert.strictEqual(req.destroyed, true);
+    server.close();
+  });
 }
diff --git a/test/parallel/test-http-client-set-timeout.js b/test/parallel/test-http-client-set-timeout.js
@@ -38,6 +38,7 @@ server.listen(0, mustCall(() => {
   }));
 
   req.on('close', mustCall(() => {
+    strictEqual(req.destroyed, true);
     server.close();
   }));
 

diff --git a/test/parallel/test-http-client-timeout-event.js b/test/parallel/test-http-client-timeout-event.js
@@ -21,6 +21,7 @@
 
 'use strict';
 const common = require('../common');
+const assert = require('assert');
 const http = require('http');
 
 const options = {
@@ -38,7 +39,10 @@ server.listen(0, options.host, function() {
   req.on('error', function() {
     // This space is intentionally left blank
   });
-  req.on('close', common.mustCall(() => server.close()));
+  req.on('close', common.mustCall(() => {
+    assert.strictEqual(req.destroyed, true);
+    server.close();
+  }));
 
   req.setTimeout(1);
   req.on('timeout', common.mustCall(() => {

diff --git a/test/parallel/test-http-client-timeout-option.js b/test/parallel/test-http-client-timeout-option.js
@@ -23,7 +23,10 @@ server.listen(0, options.host, function() {
   req.on('error', function() {
     // This space is intentionally left blank
   });
-  req.on('close', common.mustCall(() => server.close()));
+  req.on('close', common.mustCall(() => {
+    assert.strictEqual(req.destroyed, true);
+    server.close();
+  }));
 
   let timeout_events = 0;
   req.on('timeout', common.mustCall(() => timeout_events += 1));

diff --git a/test/parallel/test-http-client-timeout.js b/test/parallel/test-http-client-timeout.js
@@ -41,6 +41,7 @@ server.listen(0, options.host, function() {
     // This space intentionally left blank
   });
   req.on('close', function() {
+    assert.strictEqual(req.destroyed, true);
     server.close();
   });
   function destroy() {

diff --git a/test/parallel/test-http-keepalive-free.js b/test/parallel/test-http-keepalive-free.js
@@ -0,0 +1,36 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+
+const http = require('http');
+
+for (const method of ['abort', 'destroy']) {
+  const server = http.createServer(common.mustCall((req, res) => {
+    res.end(req.url);
+  }));
+  server.listen(0, common.mustCall(() => {
+    const agent = http.Agent({ keepAlive: true });
+
+    const req = http
+      .request({
+        port: server.address().port,
+        agent
+      })
+      .on('socket', common.mustCall((socket) => {
+        socket.on('free', common.mustCall());
+      }))
+      .on('response', common.mustCall((res) => {
+        assert.strictEqual(req.destroyed, false);
+        res.on('end', () => {
+          assert.strictEqual(req.destroyed, true);
+          req[method]();
+          assert.strictEqual(req.socket.destroyed, false);
+          agent.destroy();
+          server.close();
+        }).resume();
+      }))
+      .end();
+    assert.strictEqual(req.destroyed, false);
+  }));
+}
-Original file line number
+Diff line change
@@ Expand Up / @@ -38,6 +38,7 @@ server.listen(0, mustCall(() => { @@
       }));
       req.on('close', mustCall(() => {
+        strictEqual(req.destroyed, true);
         server.close();
       }));
@@ Expand Down @@