Skip to content

http: fix keylog listener setup on existing agent sockets - #65066

Open
shani-singh1 wants to merge 1 commit into
nodejs:mainfrom
shani-singh1:http-agent-keylog-existing-sockets
Open

http: fix keylog listener setup on existing agent sockets#65066
shani-singh1 wants to merge 1 commit into
nodejs:mainfrom
shani-singh1:http-agent-keylog-existing-sockets

Conversation

@shani-singh1

Copy link
Copy Markdown

maybeEnableKeylog() runs as the agent's 'newListener' handler and attaches the agent's keylog handler to the sockets the agent already owns:

// Existing sockets will start listening on keylog now.
const sockets = ObjectValues(this.sockets);
for (let i = 0; i < sockets.length; i++) {
  sockets[i].on('keylog', this[kOnKeylog]);
}

agent.sockets maps a name to an array of sockets, so ObjectValues() yields arrays, not sockets, and .on() is called on an array. Agent.prototype.destroy() in the same file gets this right with a nested walk over both maps.

Two things follow. Adding a 'keylog' listener to an agent that already owns a socket throws, and because the throw happens inside the 'newListener' handler it propagates out of agent.on() before the listener is stored, so the caller gets an exception and no listener. Separately, agent.freeSockets is never visited, so idle keep-alive sockets never start listening even once the crash is out of the way.

Reproduction

const http = require('http');

const server = http.createServer((req, res) => setTimeout(() => res.end('ok'), 300));

server.listen(0, '127.0.0.1', () => {
  const agent = new http.Agent({ keepAlive: true });
  const req = http.get({ host: '127.0.0.1', port: server.address().port, agent }, (res) => res.resume());
  req.on('error', () => {});
  req.on('socket', () => setImmediate(() => {
    console.log('bucket is an Array :', Array.isArray(Object.values(agent.sockets)[0]));
    try {
      agent.on('keylog', () => {});
      console.log('agent.on("keylog") : OK');
    } catch (err) {
      console.log('agent.on("keylog") : THREW ' + err.constructor.name + ': ' + err.message);
    }
    console.log('keylog listeners   :', agent.listenerCount('keylog'), '(expected 1)');
    agent.destroy();
    server.close();
    process.exit(0);
  }));
});

On v24.11.1:

bucket is an Array : true
agent.on("keylog") : THREW TypeError: sockets[i].on is not a function
keylog listeners   : 0 (expected 1)

The documented 'keylog' event on https.Agent is therefore unusable on any agent that has already opened a socket, which is the normal case for a long-lived agent. The existing coverage in test/parallel/test-https-agent-keylog.js registers the listener on https.globalAgent before any request is made, so agent.sockets is empty and the loop body never runs.

Change

Walk both maps the same way Agent.prototype.destroy() does.

Verification

Running the old and new loop bodies against a real http.Agent holding one idle socket and one in-flight socket:

sockets in freeSockets : 1
sockets in sockets     : 1
total existing sockets : 2
OLD loop               : THREW TypeError: sockets[i].on is not a function
NEW loop               : no throw
sockets wired by NEW   : 2 / 2  (expected all)

The added test uses two servers so the sockets get different names and one stays parked in freeSockets rather than being reused. It fails on main with the TypeError above at the agent.on('keylog', ...) line, and passes with this change.

`maybeEnableKeylog()` runs as the agent's `'newListener'` handler and
attaches the agent's keylog handler to the sockets the agent already
owns. `agent.sockets` maps a name to an array of sockets, but the loop
treated those arrays as sockets and called `.on()` on them.

Adding a `'keylog'` listener to an agent that already owned a socket
therefore threw `TypeError: sockets[i].on is not a function` out of
`agent.on('keylog', ...)`. Since the throw happened inside the
`'newListener'` handler it propagated before the listener was stored,
so the caller got an exception and no listener. Sockets parked in
`agent.freeSockets` were never visited at all.

Walk both maps the way `Agent.prototype.destroy()` does.
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants