http: don't destroy socket after request completes

Aborting a ClientRequest after the request has finished sending
and the response has fully arrived has nothing left to cancel.
destroy() still called socket.destroy(err), but the resulting
'error' is emitted on a later tick. In that window,
responseKeepAlive() has already removed socketErrorListener while
handing the socket back to the agent's free pool, so the error
lands with no listener and crashes the process.

Skip the socket destroy when there is nothing left to cancel. This
matches the existing behavior of keepAlive: false requests, which
already drop any unread buffered response data in this situation.

Fixes: #65938
Signed-off-by: Barath <barathraj048@gmail.com>
PR-URL: #65952
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

Author: barathraj048

lib/_http_client.js

@@ -691,6 +691,14 @@ ClientRequest.prototype.destroy = function destroy(err) {
     this.res._dump();
   }
 
+  // Nothing left to cancel: the request was fully sent and the response was
+  // fully received. Destroying the socket here would emit an error on a
+  // socket that is already being released to the agent, at which point
+  // socketErrorListener has been removed and nothing would handle it.
+  if (this.writableFinished && this.res?.complete) {
+    return this;
+  }
+
   this[kError] = err;
   this.socket?.destroy(err);
 

test/parallel/test-http-client-abort-completed-keepalive.js

@@ -0,0 +1,57 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+// Aborting a request whose exchange has already completed must not destroy the
+// socket that is being released to the agent. socketErrorListener has been
+// removed by responseKeepAlive() at that point, so the error would be emitted
+// on a socket with no 'error' listener and crash the process.
+// Refs: https://github.com/nodejs/node/issues/65938
+
+const agent = new http.Agent({ keepAlive: true });
+
+const server = http.createServer((req, res) => {
+  res.end('x');
+});
+
+server.listen(0, '127.0.0.1', common.mustCall(() => {
+  const controller = new AbortController();
+
+  const req = http.get({
+    port: server.address().port,
+    host: '127.0.0.1',
+    agent,
+    signal: controller.signal,
+  }, common.mustCall(async (res) => {
+    res.on('error', common.mustNotCall());
+
+    for await (const chunk of res) {
+      assert.strictEqual(chunk.length, 1);
+      assert.strictEqual(res.complete, true);
+      assert.strictEqual(req.writableFinished, true);
+      controller.abort(new Error('stop reading'));
+      break;
+    }
+
+    // The socket must survive the abort and go back to the pool, and a
+    // subsequent request must be able to reuse it.
+    const res2 = await new Promise((resolve, reject) => {
+      const req2 = http.get({
+        port: server.address().port,
+        host: '127.0.0.1',
+        agent,
+      }, resolve);
+      req2.on('error', reject);
+    });
+
+    let body = '';
+    for await (const chunk of res2) body += chunk;
+    assert.strictEqual(body, 'x');
+
+    agent.destroy();
+    server.close();
+  }));
+
+  req.on('error', common.mustNotCall());
+}));