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

PR-URL: #33120
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

Author: ronag

lib/_http_client.js

@@ -415,6 +415,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.
     // TOOD(ronag): res.destroy(err)
@@ -673,7 +675,9 @@ function responseKeepAlive(req) {
   // handlers have a chance to run.
   defaultTriggerAsyncIdScope(asyncId, process.nextTick, emitFreeNT, req);
 
+  req.destroyed = true;
   if (req.res) {
+    req.res.destroyed = true;
     // Detach socket from IncomingMessage to avoid destroying the freed
     // socket in IncomingMessage.destroy().
     req.res.socket = null;
@@ -719,7 +723,6 @@ function requestOnPrefinish() {
 function emitFreeNT(req) {
   req.emit('close');
   if (req.res) {
-    req.res.destroyed = true;
     req.res.emit('close');
   }
 

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();
     }))

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();
         }))

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();
   }));

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();
+  });
 }

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();
   }));
 

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(() => {

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));

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() {

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);
+  }));
+}