http: set IncomingMessage.destroyed

IncomingMessage is a Readable stream and should properly set the destroyed property. PR-URL: #33131 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
nodejs · May 11, 2020 · b634d4b · b634d4b
1 parent 736ca65
commit b634d4b
Show file tree

Hide file tree

Showing 10 changed files with 94 additions and 4 deletions.
diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -422,10 +422,12 @@ function socketCloseListener() {
     req.emit('close');
     if (!res.aborted && res.readable) {
       res.on('end', function() {
+        this.destroyed = true;
         this.emit('close');
       });
       res.push(null);
     } else {
+      res.destroyed = true;
       res.emit('close');
     }
   } else {
@@ -539,6 +541,7 @@ function socketOnData(d) {
       socket.readableFlowing = null;
 
       req.emit(eventName, res, socket, bodyHead);
+      req.destroyed = true;
       req.emit('close');
     } else {
       // Requested Upgrade or used CONNECT method, but have no handler.
@@ -710,6 +713,7 @@ function requestOnPrefinish() {
 function emitFreeNT(req) {
   req.emit('close');
   if (req.res) {
+    req.res.destroyed = true;
     req.res.emit('close');
   }
 

diff --git a/lib/_http_incoming.js b/lib/_http_incoming.js
@@ -119,6 +119,8 @@ IncomingMessage.prototype._read = function _read(n) {
 // any messages, before ever calling this.  In that case, just skip
 // it, since something else is destroying this connection anyway.
 IncomingMessage.prototype.destroy = function destroy(error) {
+  // TODO(ronag): Implement in terms of _destroy
+  this.destroyed = true;
   if (this.socket)
     this.socket.destroy(error);
 };

diff --git a/lib/_http_server.js b/lib/_http_server.js
@@ -205,7 +205,10 @@ function onServerResponseClose() {
   // Ergo, we need to deal with stale 'close' events and handle the case
   // where the ServerResponse object has already been deconstructed.
   // Fortunately, that requires only a single if check. :-)
-  if (this._httpMessage) this._httpMessage.emit('close');
+  if (this._httpMessage) {
+    this._httpMessage.destroyed = true;
+    this._httpMessage.emit('close');
+  }
 }
 
 ServerResponse.prototype.assignSocket = function assignSocket(socket) {
@@ -534,6 +537,7 @@ function abortIncoming(incoming) {
   while (incoming.length) {
     const req = incoming.shift();
     req.aborted = true;
+    req.destroyed = true;
     req.emit('aborted');
     req.emit('close');
   }
@@ -660,11 +664,13 @@ function clearIncoming(req) {
   if (parser && parser.incoming === req) {
     if (req.readableEnded) {
       parser.incoming = null;
+      req.destroyed = true;
       req.emit('close');
     } else {
       req.on('end', clearIncoming);
     }
   } else {
+    req.destroyed = true;
     req.emit('close');
   }
 }
@@ -708,6 +714,7 @@ function resOnFinish(req, res, socket, state, server) {
 }
 
 function emitCloseNT(self) {
+  self.destroyed = true;
   self.emit('close');
 }
 

diff --git a/test/parallel/test-http-client-res-destroyed.js b/test/parallel/test-http-client-res-destroyed.js
@@ -0,0 +1,41 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+{
+  const server = http.createServer(common.mustCall((req, res) => {
+    res.end('asd');
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    http.get({
+      port: server.address().port
+    }, common.mustCall((res) => {
+      assert.strictEqual(res.destroyed, false);
+      res.destroy();
+      assert.strictEqual(res.destroyed, true);
+      res.on('close', common.mustCall(() => {
+        server.close();
+      }));
+    }));
+  }));
+}
+
+{
+  const server = http.createServer(common.mustCall((req, res) => {
+    res.end('asd');
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    http.get({
+      port: server.address().port
+    }, common.mustCall((res) => {
+      assert.strictEqual(res.destroyed, false);
+      res.on('close', common.mustCall(() => {
+        assert.strictEqual(res.destroyed, true);
+        server.close();
+      })).resume();
+    }));
+  }));
+}
diff --git a/test/parallel/test-http-connect-req-res.js b/test/parallel/test-http-connect-req-res.js
@@ -33,7 +33,10 @@ server.listen(0, common.mustCall(function() {
     path: 'example.com:443'
   }, common.mustNotCall());
 
-  req.on('close', common.mustCall());
+  assert.strictEqual(req.destroyed, false);
+  req.on('close', common.mustCall(() => {
+    assert.strictEqual(req.destroyed, true);
+  }));
 
   req.on('connect', common.mustCall(function(res, socket, firstBodyChunk) {
     console.error('Client got CONNECT request');

diff --git a/test/parallel/test-http-connect.js b/test/parallel/test-http-connect.js
@@ -62,7 +62,10 @@ server.listen(0, common.mustCall(() => {
     assert.strictEqual(socket._httpMessage, req);
   }));
 
-  req.on('close', common.mustCall());
+  assert.strictEqual(req.destroyed, false);
+  req.on('close', common.mustCall(() => {
+    assert.strictEqual(req.destroyed, true);
+  }));
 
   req.on('connect', common.mustCall((res, socket, firstBodyChunk) => {
     // Make sure this request got removed from the pool.

diff --git a/test/parallel/test-http-pause-resume-one-end.js b/test/parallel/test-http-pause-resume-one-end.js
@@ -22,6 +22,7 @@
 'use strict';
 const common = require('../common');
 const http = require('http');
+const assert = require('assert');
 
 const server = http.Server(function(req, res) {
   res.writeHead(200, { 'Content-Type': 'text/plain' });
@@ -43,6 +44,12 @@ server.listen(0, common.mustCall(function() {
       });
     }));
 
-    res.on('end', common.mustCall());
+    res.on('end', common.mustCall(() => {
+      assert.strictEqual(res.destroyed, false);
+    }));
+    assert.strictEqual(res.destroyed, false);
+    res.on('close', common.mustCall(() => {
+      assert.strictEqual(res.destroyed, true);
+    }));
   }));
 }));
diff --git a/test/parallel/test-http-req-res-close.js b/test/parallel/test-http-req-res-close.js
@@ -8,13 +8,25 @@ const server = http.Server(common.mustCall((req, res) => {
   let resClosed = false;
 
   res.end();
+  let resFinished = false;
   res.on('finish', common.mustCall(() => {
+    resFinished = true;
+    assert.strictEqual(resClosed, false);
+    assert.strictEqual(res.destroyed, false);
     assert.strictEqual(resClosed, false);
   }));
+  assert.strictEqual(req.destroyed, false);
   res.on('close', common.mustCall(() => {
     resClosed = true;
+    assert.strictEqual(resFinished, true);
+    assert.strictEqual(res.destroyed, true);
+  }));
+  assert.strictEqual(req.destroyed, false);
+  req.on('end', common.mustCall(() => {
+    assert.strictEqual(req.destroyed, false);
   }));
   req.on('close', common.mustCall(() => {
+    assert.strictEqual(req.destroyed, true);
     assert.strictEqual(req._readableState.ended, true);
   }));
   res.socket.on('close', () => server.close());

diff --git a/test/parallel/test-http-response-close.js b/test/parallel/test-http-response-close.js
@@ -22,6 +22,7 @@
 'use strict';
 const common = require('../common');
 const http = require('http');
+const assert = require('assert');
 
 {
   const server = http.createServer(
@@ -39,7 +40,9 @@ const http = require('http');
           res.on('data', common.mustCall(() => {
             res.destroy();
           }));
+          assert.strictEqual(res.destroyed, false);
           res.on('close', common.mustCall(() => {
+            assert.strictEqual(res.destroyed, true);
             server.close();
           }));
         })
@@ -61,7 +64,12 @@ const http = require('http');
       http.get(
         { port: server.address().port },
         common.mustCall((res) => {
+          assert.strictEqual(res.destroyed, false);
+          res.on('end', common.mustCall(() => {
+            assert.strictEqual(res.destroyed, false);
+          }));
           res.on('close', common.mustCall(() => {
+            assert.strictEqual(res.destroyed, true);
             server.close();
           }));
           res.resume();

diff --git a/test/parallel/test-http-server-stale-close.js b/test/parallel/test-http-server-stale-close.js
@@ -23,6 +23,7 @@
 require('../common');
 const http = require('http');
 const fork = require('child_process').fork;
+const assert = require('assert');
 
 if (process.env.NODE_TEST_FORK_PORT) {
   const req = http.request({
@@ -37,7 +38,9 @@ if (process.env.NODE_TEST_FORK_PORT) {
   const server = http.createServer((req, res) => {
     res.writeHead(200, { 'Content-Length': '42' });
     req.pipe(res);
+    assert.strictEqual(req.destroyed, false);
     req.on('close', () => {
+      assert.strictEqual(req.destroyed, true);
       server.close();
       res.end();
     });