http: avoid aborting IncomingMessage signal on normal close

IncomingMessage 'close' is emitted when request handling completes, not
only when the underlying connection is closed. Using that event to abort
IncomingMessage.signal makes the signal abort after a request body is
read normally.

Track the underlying socket close instead, and detach the listener when
the request or response completes normally. This keeps the signal from
being aborted by normal stream completion or later keep-alive socket
closure, while still aborting it when the connection closes during the
active request lifecycle.

Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
PR-URL: #64392
Fixes: #64390
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>

Author: Archkon

doc/api/http.md

@@ -3010,13 +3010,20 @@ Calls `message.socket.setTimeout(msecs, callback)`.
 
 <!-- YAML
 added: v24.16.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/64392
+    description: The signal is no longer aborted after the message
+                 completes normally.
 -->
 
 * Type: {AbortSignal}
 
-An {AbortSignal} that is aborted when the underlying socket closes or the
-request is destroyed. The signal is created lazily on first access — no
-{AbortController} is allocated for requests that never use this property.
+An {AbortSignal} that is aborted when the message is destroyed before
+completion or when its underlying socket closes before request handling or
+response reading completes.
+The signal is created lazily on first access — no {AbortController} is allocated
+for requests that never use this property.
 
 This is useful for cancelling downstream asynchronous work such as database
 queries or `fetch` calls when a client disconnects mid-request.

lib/_http_client.js

@@ -50,6 +50,7 @@ const {
   prepareError,
   kSkipPendingData,
 } = require('_http_common');
+const { kDetachAbortSignal } = require('_http_incoming');
 const {
   kHighWaterMark,
   kUniqueHeaders,
@@ -1017,6 +1018,8 @@ function responseOnEnd() {
   const req = this.req;
   const socket = req.socket;
 
+  this[kDetachAbortSignal]();
+
   if (socket) {
     if (req.timeoutCb) socket.removeListener('timeout', emitRequestTimeout);
     socket.removeListener('timeout', responseOnTimeout);

lib/_http_incoming.js

@@ -38,6 +38,11 @@ const kTrailers = Symbol('kTrailers');
 const kTrailersDistinct = Symbol('kTrailersDistinct');
 const kTrailersCount = Symbol('kTrailersCount');
 const kAbortController = Symbol('kAbortController');
+const kAbortSignalSocket = Symbol('kAbortSignalSocket');
+const kAbortSignalListener = Symbol('kAbortSignalListener');
+const kAbortSignalDetached = Symbol('kAbortSignalDetached');
+const kAttachAbortSignal = Symbol('kAttachAbortSignal');
+const kDetachAbortSignal = Symbol('kDetachAbortSignal');
 
 function readStart(socket) {
   if (socket && !socket._paused && socket.readable)
@@ -94,6 +99,9 @@ function IncomingMessage(socket) {
   // read by the user, so there's no point continuing to handle it.
   this._dumped = false;
   this[kAbortController] = null;
+  this[kAbortSignalSocket] = null;
+  this[kAbortSignalListener] = null;
+  this[kAbortSignalDetached] = false;
 }
 ObjectSetPrototypeOf(IncomingMessage.prototype, Readable.prototype);
 ObjectSetPrototypeOf(IncomingMessage, Readable);
@@ -195,18 +203,51 @@ ObjectDefineProperty(IncomingMessage.prototype, 'signal', {
     if (this[kAbortController] === null) {
       const ac = new AbortController();
       this[kAbortController] = ac;
-      if (this.destroyed) {
+      if (this.destroyed && (!this.readableEnded || !this.complete)) {
         ac.abort();
       } else {
-        this.once('close', function() {
-          ac.abort();
-        });
+        this[kAttachAbortSignal]();
       }
     }
     return this[kAbortController].signal;
   },
 });
 
+IncomingMessage.prototype[kAttachAbortSignal] = function() {
+  if (this[kAbortController].signal.aborted ||
+      this[kAbortSignalDetached] ||
+      this[kAbortSignalListener] !== null) {
+    return;
+  }
+
+  const socket = this.socket;
+  if (!socket) {
+    return;
+  }
+
+  if (socket.destroyed) {
+    abortSignal(this);
+    return;
+  }
+
+  this[kAbortSignalSocket] = socket;
+  this[kAbortSignalListener] = () => {
+    abortSignal(this);
+  };
+  socket.once('close', this[kAbortSignalListener]);
+};
+
+IncomingMessage.prototype[kDetachAbortSignal] = function() {
+  const socket = this[kAbortSignalSocket];
+  const listener = this[kAbortSignalListener];
+  this[kAbortSignalDetached] = true;
+  this[kAbortSignalSocket] = null;
+  this[kAbortSignalListener] = null;
+  if (socket !== null && listener !== null) {
+    socket.removeListener('close', listener);
+  }
+};
+
 IncomingMessage.prototype.setTimeout = function setTimeout(msecs, callback) {
   if (callback)
     this.on('timeout', callback);
@@ -234,6 +275,7 @@ IncomingMessage.prototype._destroy = function _destroy(err, cb) {
   if (!this.readableEnded || !this.complete) {
     this.aborted = true;
     this.emit('aborted');
+    abortSignal(this);
   }
 
   // If aborted and the underlying socket is not already destroyed,
@@ -255,6 +297,13 @@ IncomingMessage.prototype._destroy = function _destroy(err, cb) {
   }
 };
 
+function abortSignal(self) {
+  self[kDetachAbortSignal]();
+  if (self[kAbortController] !== null) {
+    self[kAbortController].abort();
+  }
+}
+
 IncomingMessage.prototype._addHeaderLines = _addHeaderLines;
 function _addHeaderLines(headers, n) {
   if (headers?.length) {
@@ -472,6 +521,7 @@ function onError(self, error, cb) {
 
 module.exports = {
   IncomingMessage,
+  kDetachAbortSignal,
   readStart,
   readStop,
 };

lib/_http_server.js

@@ -68,7 +68,10 @@ const {
   defaultTriggerAsyncIdScope,
   getOrSetAsyncId,
 } = require('internal/async_hooks');
-const { IncomingMessage } = require('_http_incoming');
+const {
+  IncomingMessage,
+  kDetachAbortSignal,
+} = require('_http_incoming');
 const {
   ConnResetException,
   codes: {
@@ -1105,6 +1108,7 @@ function resOnFinish(req, res, socket, state, server) {
   // array will be empty.
   assert(state.incoming.length === 0 || state.incoming[0] === req);
 
+  req[kDetachAbortSignal]();
   state.incoming.shift();
 
   // If the user never called req.read(), and didn't pipe() or

test/parallel/test-http-request-signal.js

@@ -4,7 +4,7 @@ const common = require('../common');
 const assert = require('assert');
 const http = require('http');
 
-// Test 1: req.signal is an AbortSignal and aborts on 'close'
+// Test 1: req.signal is an AbortSignal and aborts on socket close
 {
   const server = http.createServer(common.mustCall((req, res) => {
     assert.ok(req.signal instanceof AbortSignal);
@@ -21,21 +21,68 @@ const http = require('http');
   }));
 }
 
-// Test 2: req.signal is aborted if accessed after destroy
+// Test 2: req.signal is not aborted when a request body completes normally.
+{
+  const body = JSON.stringify({ hello: 'world' });
+  const server = http.createServer(common.mustCall((req, res) => {
+    assert.ok(req.signal instanceof AbortSignal);
+    assert.strictEqual(req.signal.aborted, false);
+    req.signal.onabort = common.mustNotCall();
+
+    req.on('close', common.mustCall(() => {
+      assert.strictEqual(req.aborted, false);
+      assert.strictEqual(req.complete, true);
+      assert.strictEqual(req.signal.aborted, false);
+    }));
+
+    req.on('end', common.mustCall(() => {
+      setTimeout(common.mustCall(() => {
+        assert.strictEqual(req.aborted, false);
+        assert.strictEqual(req.complete, true);
+        assert.strictEqual(req.signal.aborted, false);
+        res.end('ok');
+      }), 10);
+    }));
+    req.resume();
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    const clientReq = http.request(
+      {
+        port: server.address().port,
+        method: 'PATCH',
+        path: '/tables/1',
+        headers: {
+          'content-type': 'application/json',
+          'content-length': Buffer.byteLength(body),
+        },
+      },
+      common.mustCall((res) => {
+        res.resume();
+        res.on('end', common.mustCall(() => {
+          server.close();
+        }));
+      }),
+    );
+    clientReq.end(body);
+  }));
+}
+
+// Test 3: req.signal is aborted if accessed after destroy
 {
   const req = new http.IncomingMessage(null);
   req.destroy();
   assert.strictEqual(req.signal.aborted, true);
 }
 
-// Test 3: Multiple accesses return the same signal
+// Test 4: Multiple accesses return the same signal
 {
   const req = new http.IncomingMessage(null);
   assert.strictEqual(req.signal, req.signal);
 }
 
 
-// Test 4: res.signal on a client-side http.request() response (IncomingMessage).
+// Test 5: res.signal on a client-side http.request() response (IncomingMessage).
 {
   const server = http.createServer(common.mustCall((req, res) => {
     res.writeHead(200);
@@ -61,7 +108,36 @@ const http = require('http');
   }));
 }
 
-// Test 5: Client cancels a pending request.
+// Test 6: res.signal is not aborted when a response body completes normally.
+{
+  const server = http.createServer(common.mustCall((req, res) => {
+    res.end('ok');
+  }));
+
+  server.listen(0, common.mustCall(() => {
+    const clientReq = http.request(
+      { port: server.address().port },
+      common.mustCall((res) => {
+        assert.ok(res.signal instanceof AbortSignal);
+        assert.strictEqual(res.signal.aborted, false);
+        res.signal.onabort = common.mustNotCall();
+
+        res.resume();
+        res.on('end', common.mustCall(() => {
+          assert.strictEqual(res.complete, true);
+          assert.strictEqual(res.signal.aborted, false);
+        }));
+        res.on('close', common.mustCall(() => {
+          assert.strictEqual(res.signal.aborted, false);
+          server.close();
+        }));
+      }),
+    );
+    clientReq.end();
+  }));
+}
+
+// Test 7: Client cancels a pending request.
 {
   const server = http.createServer(common.mustCall((req, res) => {
     req.signal.onabort = common.mustCall(() => {