Reduce likelihood of race conditions on keep-alive timeout calculatio… #52653
Conversation
|
Review requested:
|
nodejs-github-bot
added
http
|
ping @mweberxyz |
github-actions
bot
removed
the
request-ci
lib/_http_agent.js
Outdated
| // Let the timer expires before the announced timeout to reduce | ||
| // the likelihood of ECONNRESET errors | ||
| let serverHintTimeout = ( NumberParseInt(hint) * 1000 ) - 1000; | ||
| serverHintTimeout = serverHintTimeout > 0 ? serverHintTimeout : 0; | ||
|
|
||
| if (serverHintTimeout < agentTimeout) { |
There was a problem hiding this comment.
This conditional needs to be fixed - agentTimeout defaults to 0, so the serverHintTimeout is never being set.
There was a problem hiding this comment.
In this case the socket shouldn't be reused at all. Is setting socket.setTimeout(0) the right way to do it?
There was a problem hiding this comment.
Take a look at socket docs - socket.setTimeout(0) doesn't mean immediate, it means never.
There was a problem hiding this comment.
Sorry, I have limited understanding of the network internals. In undici ( https://github.com/nodejs/undici/pull/291/files ) if the keepAliveTimeout goes down to 0 they flag the connection as reset:
if (!keepAliveTimeout || keepAliveTimeout < 1e3) {
client[kReset] = true
}
I don't know how to achieve the same result here. Maybe:
socket.setKeepAlive(false);
?
There was a problem hiding this comment.
I also have a limited understanding of network internals, but I think:
if (!agentTimeout || serverHintTimeout < agentTimeout) {
is what you want.
| socket.setTimeout(server.keepAliveTimeout); | ||
| // Increase the internal timeout wrt the advertised value to reduce likeliwood of ECONNRESET errors | ||
| // due to race conditions between the client and server timeout calculation | ||
| socket.setTimeout(server.keepAliveTimeout + 1000); |
There was a problem hiding this comment.
I don't think this server change is necessary.
There was a problem hiding this comment.
Might be the cause of all the test failures.
There was a problem hiding this comment.
This fix protects clients that are using the hint timeout as is (current node impl) without adjusting it for network (or cpu load) delays
There was a problem hiding this comment.
If server.keepAliveTimeout is set to 0 (never time out) this will change it to 1 second.
There was a problem hiding this comment.
If server.keepAliveTimeout == 0 then the if condition line1012 is false. The timeout on the socket is set only if server.keepAliveTimeout != 0
There was a problem hiding this comment.
You're correct, sorry I missed that.
lib/_http_agent.js
Outdated
| const serverHintTimeout = NumberParseInt(hint) * 1000; | ||
| // Let the timer expires before the announced timeout to reduce | ||
| // the likelihood of ECONNRESET errors | ||
| let serverHintTimeout = ( NumberParseInt(hint) * 1000 ) - 1000; |
There was a problem hiding this comment.
In the case the server responds with a 1 second keepalive, this will set the disable the timeout because serverHintTimeout will be 0 -- maybe go with - 500 in place of - 1000 ?
| serverHintTimeout = serverHintTimeout > 0 ? serverHintTimeout : 0; | ||
| if (serverHintTimeout === 0) { | ||
| // cannot safely reuse the socket because the server timeout is too short | ||
| canKeepSocketAlive = false; |
There was a problem hiding this comment.
I have updated the PR to just skip keep alive if the timeout is too short. In my local build the test now pass.
This comment has been minimized.
This comment has been minimized.
zanettea
changed the title
zanettea
force-pushed
the
main
branch
2 times, most recently
from
8407fd7
to
bc34680
Compare
|
Found the same issue in dotnet -- they went with a 1 second offset as well, so that was a good choice. 👍 |
zanettea
force-pushed
the
main
branch
3 times, most recently
from
ba0d895
to
2124b55
Compare
reduce likelihood of race conditions on keep-alive timeout calculation between http1.1 servers and clients and honor server keep-alive timeout when agentTimeout is not set Fixes: nodejs#47130 Fixes: nodejs#52649
Added 1 seconds threshold in keepalive timeout client-side
Added 1 second threshold in keepalive timeout server-side (expire the socket timeout 1 sec after the announced timeout)
Probably better to use a configurable threshold like in undici keepAliveTimeoutThreshold (nodejs/undici#291)