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

Reduce likelihood of race conditions on keep-alive timeout calculatio… #52653

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions lib/_http_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) {
socket.unref();

let agentTimeout = this.options.timeout || 0;
let canKeepSocketAlive = true;

if (socket._httpMessage?.res) {
const keepAliveHint = socket._httpMessage.res.headers['keep-alive'];
Expand All @@ -496,9 +497,14 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) {
const hint = RegExpPrototypeExec(/^timeout=(\d+)/, keepAliveHint)?.[1];

if (hint) {
const serverHintTimeout = NumberParseInt(hint) * 1000;

if (serverHintTimeout < agentTimeout) {
// Let the timer expires before the announced timeout to reduce
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Let the timer expires before the announced timeout to reduce
// Let the timer expire before the announced timeout to reduce

// the likelihood of ECONNRESET errors
let serverHintTimeout = (NumberParseInt(hint) * 1000) - 1000;
serverHintTimeout = serverHintTimeout > 0 ? serverHintTimeout : 0;
if (serverHintTimeout === 0) {
// Cannot safely reuse the socket because the server timeout is too short
canKeepSocketAlive = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the PR to just skip keep alive if the timeout is too short. In my local build the test now pass.

} else if (!agentTimeout || serverHintTimeout < agentTimeout) {
agentTimeout = serverHintTimeout;
}
}
Expand All @@ -509,7 +515,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) {
socket.setTimeout(agentTimeout);
}

return true;
return canKeepSocketAlive;
};

Agent.prototype.reuseSocket = function reuseSocket(socket, req) {
Expand Down
4 changes: 3 additions & 1 deletion lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -1010,7 +1010,9 @@ function resOnFinish(req, res, socket, state, server) {
}
} else if (state.outgoing.length === 0) {
if (server.keepAliveTimeout && typeof socket.setTimeout === 'function') {
socket.setTimeout(server.keepAliveTimeout);
// Increase the internal timeout wrt the advertised value to reduce likeliwood of ECONNRESET errors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Increase the internal timeout wrt the advertised value to reduce likeliwood of ECONNRESET errors
// Increase the internal timeout wrt the advertised value to reduce likelihood of ECONNRESET errors

// due to race conditions between the client and server timeout calculation
socket.setTimeout(server.keepAliveTimeout + 1000);

Choose a reason for hiding this comment

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

I don't think this server change is necessary.

Choose a reason for hiding this comment

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

Might be the cause of all the test failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix protects clients that are using the hint timeout as is (current node impl) without adjusting it for network (or cpu load) delays

Choose a reason for hiding this comment

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

If server.keepAliveTimeout is set to 0 (never time out) this will change it to 1 second.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If server.keepAliveTimeout == 0 then the if condition line1012 is false. The timeout on the socket is set only if server.keepAliveTimeout != 0

Choose a reason for hiding this comment

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

You're correct, sorry I missed that.

state.keepAliveTimeoutSet = true;
}
} else {
Expand Down