Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add agent socket reuse #15

Merged
merged 10 commits into from
Aug 14, 2024
Merged

Add agent socket reuse #15

merged 10 commits into from
Aug 14, 2024

Conversation

yassernasc
Copy link
Contributor

@yassernasc yassernasc commented Jul 11, 2024

Not included on this first draft:

  • request pool (maxSockets, maxTotalSockets and maxTotalSockets aren't being used yet)
  • 'agentRemove' event
  • agent.reuseSocket request parameter added
  • scheduling option
  • agent timeout support

What is included:

  • agent.keepSocketAlive
  • agent.getName (minimal version)
  • socket reuse with agent.getSocket and socket 'free' event
  • agent.destroy

Extra thing: because http.request by default uses the global agent, which enables keepAlive by default, it made some previous tests start to get stuck, the solution was to add agent: false option.

@yassernasc yassernasc requested a review from kasperisager July 11, 2024 14:23
@yassernasc yassernasc marked this pull request as draft July 11, 2024 14:43
@yassernasc yassernasc marked this pull request as ready for review July 12, 2024 19:29
Copy link
Contributor

@kasperisager kasperisager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid start! 👌 The tests stalling without agent: false smells like a missing unref() somewhere so that's worth looking into; they should pass just fine even with the global agent.

lib/agent.js Outdated Show resolved Hide resolved
lib/agent.js Outdated Show resolved Hide resolved
lib/agent.js Outdated Show resolved Hide resolved
@yassernasc
Copy link
Contributor Author

The tests stalling without agent: false smells like a missing unref() somewhere so that's worth looking into; they should pass just fine even with the global agent.

Maybe it's missing somewhere at bare-tcp package layer because the unref seems to be called only at the agent.keepSocketAlive on nodejs agent implementation.

@yassernasc yassernasc requested a review from kasperisager July 15, 2024 18:31
@kasperisager
Copy link
Contributor

Looking into the HTTP server implementation in Node.js, it explicitly closes idle connections when closing the server: https://github.com/nodejs/node/blob/362afa52ebe462a39874915e5e70d261db153c58/lib/_http_server.js#L592-L606. The lack of that is most likely what causes the tests to stall.

lib/agent.js Outdated Show resolved Hide resolved
lib/agent.js Outdated Show resolved Hide resolved
lib/agent.js Outdated Show resolved Hide resolved
lib/agent.js Outdated Show resolved Hide resolved
lib/agent.js Outdated Show resolved Hide resolved
lib/agent.js Outdated Show resolved Hide resolved
lib/agent.js Outdated Show resolved Hide resolved
lib/client-request.js Outdated Show resolved Hide resolved
@yassernasc yassernasc force-pushed the socket-reuse branch 3 times, most recently from ab95cda to f8951fd Compare July 18, 2024 01:21
@yassernasc
Copy link
Contributor Author

I had a hard time making the CI work after the _closeConnections implementation, the only test that still needs agent: false is the server and client do big writes because it hangs on linux, the request ‘close’ event doesn't seem to be triggered;

At server._closeConnections, I used socket.end instead socket.destroy for some problems with tests too.

@yassernasc yassernasc requested a review from kasperisager July 18, 2024 02:02
lib/agent.js Outdated Show resolved Hide resolved
lib/client-request.js Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@yassernasc yassernasc requested a review from kasperisager July 19, 2024 13:23
Copy link
Contributor

@kasperisager kasperisager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tweaked the last bits so this should be ready to land now 👏

@kasperisager kasperisager merged commit 09b0f25 into main Aug 14, 2024
3 checks passed
@kasperisager kasperisager deleted the socket-reuse branch August 14, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants