Review requested:

• @nodejs/http
• @nodejs/net

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.13%. Comparing base (8c1d128) to head (a008632).
⚠️ Report is 20 commits behind head on main.

@@           Coverage Diff           @@
##             main   #64653   +/-   ##
=======================================
  Coverage   90.12%   90.13%           
=======================================
  Files         741      741           
  Lines      242091   242142   +51     
  Branches    45563    45563           
=======================================
+ Hits       218180   218250   +70     
+ Misses      15430    15394   -36     
- Partials     8481     8498   +17     

CI: https://ci.nodejs.org/job/node-test-pull-request/74993/

@pimterry Thanks for the callout — you were right. I investigated the agent socket reuse case and confirmed both a deadlock and a functional bug.

1. Deadlock with reused socket

For the deadlock to occur, three conditions must align:

1. Write size > OM's kHighWaterMark (so write() returns false, kNeedDrain set)
2. Write size < reused socket's writableHighWaterMark (so socket.write() returns true, socket never emits drain)
3. Write size > kernel TCP send buffer (so socket.writableLength > 0 when _flush() checks, preventing the direct drain emission at line 1204)

When all three hold, drain is never emitted from either path and the request hangs permanently.

Minimal reproduction:

const http = require('http');

// Server that delays reading — TCP send buffer fills
const server = http.createServer((req, res) => {
  setTimeout(() => { req.resume(); req.on('end', () => res.end('ok')); }, 30000);
}).listen(0, () => {
  const port = server.address().port;
  const agent = new http.Agent({ keepAlive: true });

  // Request A: creates socket with HWM=10MB
  http.request({ port, method: 'POST', agent, highWaterMark: 10 * 1024 * 1024 }, (res) => {
    res.resume();
    res.on('end', () => {
      setTimeout(() => {
        // Request B: default HWM (64KB), reuses socket (HWM=10MB)
        const req = http.request({ port, method: 'POST', agent });
        // Write 2MB: > 64KB OM, < 10MB socket, > kernel TCP buffer
        const r = req.write(Buffer.alloc(2 * 1024 * 1024));
        if (!r) {
          setTimeout(() => { console.error('DEADLOCK'); process.exit(1); }, 15000);
          req.on('drain', () => req.end());
        } else {
          req.end();
        }
      }, 100);
    });
  }).end('x');
});

# Requires reduced TCP send buffer:
sysctl -w net.ipv4.tcp_wmem="4096 16384 65536"
node repro.js  # → DEADLOCK

2. highWaterMark silently not respected (no deadlock required)

Even without the deadlock, the user's highWaterMark is silently ignored once the reused socket takes over (Path A):

const http = require('http');

const server = http.createServer((req, res) => {
  req.resume();
  req.on('end', () => res.end('ok'));
}).listen(0, () => {
  const port = server.address().port;
  const agent = new http.Agent({ keepAlive: true });

  // Request A: creates socket with HWM=1MB
  http.request({ port, method: 'POST', agent, highWaterMark: 1024 * 1024 }, (res) => {
    res.resume();
    res.on('end', () => {
      setTimeout(() => {
        // Request B: HWM=10KB, gets reused socket (HWM=1MB)
        const req = http.request({ port, method: 'POST', agent, highWaterMark: 10 * 1024 });
        req.on('socket', () => {
          setTimeout(() => {
            console.log('Requested HWM: 10KB');
            console.log('Socket HWM:', req.socket.writableHighWaterMark); // 1MB!
            const r = req.write(Buffer.alloc(50 * 1024));
            console.log('write(50KB):', r, '— expected false, got true');
            req.end();
            req.on('response', (r2) => { r2.resume(); r2.on('end', () => server.close()); });
          }, 10);
        });
      }, 100);
    });
  }).end('x');
});

User requests 10KB backpressure but gets 1MB from the reused socket. write(50KB) returns true when it should return false.

3. Fix options

1. Sync OM's kHighWaterMark to the reused socket's value at assignment time — prevents deadlock but silently overrides the user's requested HWM.
2. Don't reuse a socket whose writableHighWaterMark differs from the request's highWaterMark — agent creates a fresh socket instead.

Changing a pre-existing socket's HWM is not feasible — writableHighWaterMark on a connected TCP socket is effectively read-only (the underlying kernel buffer is a separate layer not exposed via Node's TCP handle, and _writableState.highWaterMark is cosmetic since state.length stays 0 for connected sockets).

I'd recommend option 2: don't reuse if HWM differs. For requests to the same host:port, it's rare that different highWaterMark values would be used — so in practice socket reuse still happens for the vast majority of connections. In the rare case where they do differ, paying the cost of one additional TCP handshake is acceptable to ensure the user's backpressure contract is honored correctly.

If we agree on the approach, I'll raise a follow-up PR for this.

If we agree on the approach, I'll raise a follow-up PR for this.

Thanks for looking into this @trivenay, yes a follow up PR would be great.

Unfortunately, option 2 isn't possible: we can change the default agent, but users can provide any agent which might implement their own pooling in any unknown way. Similarly createConnection could return anything. I think we have to accept that in some cases socket HWM won't match the requested HWM.

I think that's OK though! Looking for more closely at the docs: this isn't actually defined explicitly as an option for http.request() at all. It's purely inherited from the Duplex options via socket.connect(), because the docs say options in socket.connect() are also supported.

In reality, many socket options will already be ignored if the socket is not managed by the request.

I think we should:

• Expand that socket.connect options text to say "...but whether these are used will depend on the behaviour of your agent and createConnection options" (i.e. no guarantees, it's only if the request actually controls the socket).
• Treat the highWaterMark as best effort: we pass it through, and use it for internal buffering when the socket is unavailable, that's it.
• Fix the internal behaviour (probably mirroring the socket HWM once it's available as you suggest in option 1, or some other approach to fix drain separately) so that this doesn't deadlock even if createConnection returns a duplex with a different highwatermark to what was requested.

Ah that said, I think mirroring the socket HWM isn't sufficient: if write() return false before the socket arrives based on the initial HWM option, we need to drain correctly even if a socket arrives with a different HWM later. We will need a separate drain fix somewhere to handle that.

@pimterry Thanks for the collaboration on this. I created a follow-up issue to align on the approach and continue the discussion there: #64680. Will raise a PR once we agree on direction.