http: fix keylog listener setup on existing agent sockets - #65066
Open
shani-singh1 wants to merge 1 commit into
Open
http: fix keylog listener setup on existing agent sockets#65066shani-singh1 wants to merge 1 commit into
shani-singh1 wants to merge 1 commit into
Conversation
`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.
Collaborator
|
Review requested:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
maybeEnableKeylog()runs as the agent's'newListener'handler and attaches the agent's keylog handler to the sockets the agent already owns:agent.socketsmaps a name to an array of sockets, soObjectValues()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 ofagent.on()before the listener is stored, so the caller gets an exception and no listener. Separately,agent.freeSocketsis never visited, so idle keep-alive sockets never start listening even once the crash is out of the way.Reproduction
On v24.11.1:
The documented
'keylog'event onhttps.Agentis therefore unusable on any agent that has already opened a socket, which is the normal case for a long-lived agent. The existing coverage intest/parallel/test-https-agent-keylog.jsregisters the listener onhttps.globalAgentbefore any request is made, soagent.socketsis 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.Agentholding one idle socket and one in-flight socket:The added test uses two servers so the sockets get different names and one stays parked in
freeSocketsrather than being reused. It fails onmainwith theTypeErrorabove at theagent.on('keylog', ...)line, and passes with this change.