http: avoid aborting IncomingMessage signal on normal close

doc/api/http.md

@@ -3045,13 +3045,20 @@ Calls `message.socket.setTimeout(msecs, callback)`.
 added:
  - v26.1.0
  - 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 {
   kUniqueHeaders,
   parseUniqueHeadersOption,
@@ -999,6 +1000,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);
@@ -242,6 +283,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,
@@ -263,6 +305,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) {
@@ -480,6 +529,7 @@ function onError(self, error, cb) {
 
 module.exports = {
   IncomingMessage,
+  kDetachAbortSignal,
   readStart,
   readStop,
 };

lib/_http_server.js

@@ -70,7 +70,10 @@ const {
   defaultTriggerAsyncIdScope,
   getOrSetAsyncId,
 } = require('internal/async_hooks');
-const { IncomingMessage } = require('_http_incoming');
+const {
+  IncomingMessage,
+  kDetachAbortSignal,
+} = require('_http_incoming');
 const {
   ConnResetException,
   codes: {
@@ -1198,6 +1201,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(() => {