http: emit drain on socket takeover and avoid stale HWM reuse

When OutgoingMessage transitions from pre-socket buffering (Path B) to
socket-connected writing (Path A), the backpressure domain changes —
subsequent writes go directly to the socket, which enforces its own
backpressure via socket.write() return values.  The OM should emit
drain at this transition point to signal that its buffer is clear and
the caller can resume writing under the socket backpressure regime.

Previously, _flush() gated drain emission on writableLength === 0
which included socket.writableLength.  This conflated two independent
backpressure domains: the OM pre-socket buffer and the socket kernel
write queue.  When the socket had a higher writableHighWaterMark than
the OM (e.g. agent-reused socket from a prior request), the socket
was never backpressured and never emitted drain, causing a permanent
deadlock.

Additionally, avoid reusing a pooled socket in http.Agent when its
writableHighWaterMark differs from the request highWaterMark, so that
the user backpressure threshold is respected for the common case of
the built-in Agent.

Signed-off-by: Naman Trivedi <trivenay@amazon.com>
Fixes: #64680
Refs: #64653
Refs: #62936

Author: trivenay

lib/_http_agent.js

@@ -392,6 +392,17 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
   const sockLen = freeLen + this.sockets[name].length;
 
   // Reusing a socket from the pool.
+  // If the caller specified a highWaterMark that differs from the pooled
+  // socket's writableHighWaterMark, skip reuse and create a fresh socket
+  // so that backpressure semantics match what the caller requested.
+  if (socket && options.highWaterMark != null &&
+      socket.writableHighWaterMark !== options.highWaterMark) {
+    debug('skip reuse, HWM mismatch (socket=%d, request=%d)',
+          socket.writableHighWaterMark, options.highWaterMark);
+    socket.destroy();
+    socket = null;
+  }
+
   if (socket) {
     asyncResetHandle(socket);
     this.reuseSocket(socket, req);

lib/_http_outgoing.js

@@ -1201,7 +1201,10 @@ OutgoingMessage.prototype._flush = function _flush() {
     if (this.finished) {
       // This is a queue to the server or client to bring in the next this.
       this._finish();
-    } else if (this[kNeedDrain] && this.writableLength === 0) {
+    } else if (this[kNeedDrain]) {
+      // _flushOutput() handed all buffered data to the socket; the OM's
+      // backpressure concern is resolved.  Subsequent writes go directly
+      // to the socket where socket-level backpressure takes over.
       this[kNeedDrain] = false;
       this.emit('drain');
     }

test/parallel/test-http-agent-highwatermark-reuse.js

@@ -0,0 +1,75 @@
+'use strict';
+
+// Regression test: when a pooled socket's writableHighWaterMark differs from
+// the new request's highWaterMark, the agent must not reuse that socket.
+// Reusing it causes the user's backpressure threshold to be silently ignored
+// (and can deadlock under TCP backpressure conditions).
+//
+// See: https://github.com/nodejs/node/pull/64653#issuecomment-5047543911
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const server = http.createServer(common.mustCall((req, res) => {
+  req.resume();
+  req.on('end', () => res.end('ok'));
+}, 3));
+
+server.listen(0, common.mustCall(() => {
+  const port = server.address().port;
+  const agent = new http.Agent({ keepAlive: true });
+
+  // Request A: creates socket with HWM=1MB.
+  http.request({
+    host: 'localhost', port, method: 'POST', agent,
+    highWaterMark: 1024 * 1024,
+  }, common.mustCall((res) => {
+    res.resume();
+    res.on('end', common.mustCall(() => {
+      // Wait for socket to return to pool.
+      setTimeout(requestB, 100);
+    }));
+  })).end('x');
+
+  function requestB() {
+    const freeCount = Object.values(agent.freeSockets).flat().length;
+    assert.strictEqual(freeCount, 1);
+
+    // Request B: HWM=10KB — must get a fresh socket, not the 1MB one.
+    const reqB = http.request({
+      host: 'localhost', port, method: 'POST', agent,
+      highWaterMark: 10 * 1024,
+    }, common.mustCall((res) => {
+      res.resume();
+      res.on('end', common.mustCall(() => {
+        setTimeout(requestC, 100);
+      }));
+    }));
+
+    reqB.on('socket', common.mustCall((socket) => {
+      assert.strictEqual(socket.writableHighWaterMark, 10 * 1024);
+    }));
+
+    reqB.end('y');
+  }
+
+  function requestC() {
+    // Request C: same HWM=10KB — should reuse the socket from Request B.
+    const reqC = http.request({
+      host: 'localhost', port, method: 'POST', agent,
+      highWaterMark: 10 * 1024,
+    }, common.mustCall((res) => {
+      res.resume();
+      res.on('end', common.mustCall(() => {
+        server.close();
+      }));
+    }));
+
+    reqC.on('socket', common.mustCall((socket) => {
+      assert.strictEqual(socket.writableHighWaterMark, 10 * 1024);
+    }));
+
+    reqC.end('z');
+  }
+}));

test/parallel/test-http-outgoing-drain-writable-length.js

@@ -37,10 +37,12 @@ const server = http.createServer(common.mustCall((req, res) => {
   assert.strictEqual(res.writableNeedDrain, true);
 
   res.on('drain', common.mustCall(() => {
-    assert.strictEqual(
-      res.writableLength, 0,
-      `'drain' fired with writableLength=${res.writableLength}`,
-    );
+    // After the changeover from pre-socket buffering to socket-connected
+    // writing, drain fires once the OM's own buffer (outputData) has been
+    // handed off to the socket.  The socket may still have data queued
+    // in libuv — that's the socket's backpressure domain, handled by
+    // subsequent write() calls returning false via Path A.
+    assert.strictEqual(res.outputSize, 0);
     res.end();
     server.close();
   }));

test/parallel/test-http-outgoing-flush-drain.js

@@ -0,0 +1,57 @@
+'use strict';
+
+// Regression test: when _flush() hands buffered data to a socket whose
+// writableHighWaterMark is higher than the OutgoingMessage's kHighWaterMark,
+// drain must still fire.  Previously, _flush() gated drain emission on
+// writableLength === 0, which included socket.writableLength — but the
+// socket was never backpressured (data < socket HWM), so drain never fired.
+//
+// See: https://github.com/nodejs/node/issues/64680
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+// Server that delays reading to keep socket.writableLength > 0 during flush.
+const server = http.createServer(common.mustCall((req, res) => {
+  setTimeout(() => {
+    req.resume();
+    req.on('end', () => res.end('ok'));
+  }, 500);
+}, 2));
+
+server.listen(0, common.mustCall(() => {
+  const port = server.address().port;
+  const agent = new http.Agent({ keepAlive: true });
+
+  // Request A: creates socket with HWM=2MB.
+  http.request({
+    host: 'localhost', port, method: 'POST', agent,
+    highWaterMark: 2 * 1024 * 1024,
+  }, common.mustCall((res) => {
+    res.resume();
+    res.on('end', common.mustCall(() => {
+      // Wait for socket to return to pool.
+      setTimeout(common.mustCall(() => {
+        // Request B: default HWM (64KB), reuses socket (HWM=2MB).
+        // Write 500KB: above OM HWM (64KB), below socket HWM (2MB).
+        const reqB = http.request({
+          host: 'localhost', port, method: 'POST', agent,
+        }, common.mustCall((res2) => {
+          res2.resume();
+          res2.on('end', common.mustCall(() => {
+            server.close();
+          }));
+        }));
+
+        const result = reqB.write(Buffer.alloc(500 * 1024));
+        assert.strictEqual(result, false);
+
+        // Drain must fire — no deadlock.
+        reqB.on('drain', common.mustCall(() => {
+          reqB.end();
+        }));
+      }), 100);
+    }));
+  })).end('x');
+}));