nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.95k stars 29.78k forks source link

Set default keepAlive options when creating HTTP/S agents #55770

Open MarioRomanDono opened 2 weeks ago

MarioRomanDono commented 2 weeks ago

What is the problem this feature will solve?

Since #37184 proposal was accepted, Node's HTTP/S globalAgent sets the keepAlive option to true by default, with a socket timeout of 5 seconds. However, I find this is inconsistent with the observed behavior when using the Agent constructor, which by default returns an agent with keepAlive set to false.

What is the feature you are proposing to solve the problem?

Set keepAlive to true when creating a new HTTP/S agent, as well as setting the socket timeout to 5 seconds, to equal the behavior of the globalAgent. However, I recognize that this change could have unintended effects for users. Additionally, the impact of setting keepAlive to true in the globalAgent is still under discussion (see #47130). So, please, check my alternatives too.

What alternatives have you considered?

  1. We can keep setting keepAlive to false by default, but in case it is provided, set a default socket timeout if timeout is not provided. I ran into this problem this week: I was using keepAlive without setting a timeout value, so sockets were not being closed after inactivity. If the server sends a FIN packet and that event is not processed by the client before sending a new request, an ECONNRESET error will be thrown (as observed in #47130). I found that setting timeout to 5 seconds, as with the globalAgent, greatly reduces the number of errors.
  2. If, for some reason, it is discouraged to set a default timeout value, at least specify in Node HTTP docs that, if it is not provided by the user, no timeout will be set (and that it could lead to possible errors as described above).

Whichever alternative is chosen, I would be more than willing to submit a PR with a solution. Thank you!

@nodejs/http (I don't know if that still works)

RedYetiDev commented 2 weeks ago

@[]()nodejs/http (I don't know if that still works)

For future reference, it does :-)