http: reduce likelihood of race conditions on keep-alive timeout

jazelly · zanettea · targos · commit 596940cfa098 · 2024-10-02T16:04:18.000+02:00
Fixes: #52649 Refs: #54293 Co-authored-by: Arrigo Zanette <zanettea@gmail.com> PR-URL: #54863 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
diff --git a/lib/_http_agent.js b/lib/_http_agent.js
@@ -49,6 +49,9 @@ const {
 const kOnKeylog = Symbol('onkeylog');
 const kRequestOptions = Symbol('requestOptions');
 const kRequestAsyncResource = Symbol('requestAsyncResource');
+
+// TODO(jazelly): make this configurable
+const HTTP_AGENT_KEEP_ALIVE_TIMEOUT_BUFFER = 1000;
 // New Agent code.
 
 // The largest departure from the previous implementation is that
@@ -473,6 +476,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'];
@@ -481,9 +485,15 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) {
 const hint = /^timeout=(\d+)/.exec(keepAliveHint)?.[1];
 
 if (hint) {
- const serverHintTimeout = NumberParseInt(hint) * 1000;
-
- if (serverHintTimeout < agentTimeout) {
+ // Let the timer expire before the announced timeout to reduce
+ // the likelihood of ECONNRESET errors
+ let serverHintTimeout = (NumberParseInt(hint) * 1000) - HTTP_AGENT_KEEP_ALIVE_TIMEOUT_BUFFER;
+ serverHintTimeout = serverHintTimeout > 0 ? serverHintTimeout : 0;
+ if (serverHintTimeout === 0) {
+ // Cannot safely reuse the socket because the server timeout is
+ // too short
+ canKeepSocketAlive = false;
+ } else if (serverHintTimeout < agentTimeout) {
 agentTimeout = serverHintTimeout;
 }
 }
@@ -494,7 +504,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) {
 socket.setTimeout(agentTimeout);
 }
 
- return true;
+ return canKeepSocketAlive;
 };
 
 Agent.prototype.reuseSocket = function reuseSocket(socket, req) {
diff --git a/lib/_http_server.js b/lib/_http_server.js
@@ -182,6 +182,8 @@ const kConnections = Symbol('http.server.connections');
 const kConnectionsCheckingInterval = Symbol('http.server.connectionsCheckingInterval');
 
 const HTTP_SERVER_TRACE_EVENT_NAME = 'http.server.request';
+// TODO(jazelly): make this configurable
+const HTTP_SERVER_KEEP_ALIVE_TIMEOUT_BUFFER = 1000;
 
 class HTTPServerAsyncResource {
 constructor(type, socket) {
@@ -998,7 +1000,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
+ // the likelihood of ECONNRESET errors.
+ socket.setTimeout(server.keepAliveTimeout + HTTP_SERVER_KEEP_ALIVE_TIMEOUT_BUFFER);
 state.keepAliveTimeoutSet = true;
 }
 } else {
diff --git a/test/parallel/test-http-keep-alive-timeout-race-condition.js b/test/parallel/test-http-keep-alive-timeout-race-condition.js
@@ -0,0 +1,38 @@
+'use strict';
+
+const common = require('../common');
+const http = require('http');
+
+const makeRequest = (port, agent) =>
+ new Promise((resolve, reject) => {
+ const req = http.get(
+ { path: '/', port, agent },
+ (res) => {
+ res.resume();
+ res.on('end', () => resolve());
+ },
+ );
+ req.on('error', (e) => reject(e));
+ req.end();
+ });
+
+const server = http.createServer(
+ { keepAliveTimeout: common.platformTimeout(2000), keepAlive: true },
+ common.mustCall((req, res) => {
+ const body = 'hello world\n';
+ res.writeHead(200, { 'Content-Length': body.length });
+ res.write(body);
+ res.end();
+ }, 2)
+);
+
+const agent = new http.Agent({ maxSockets: 5, keepAlive: true });
+
+server.listen(0, common.mustCall(async function() {
+ await makeRequest(this.address().port, agent);
+ // Block the event loop for 2 seconds
+ Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 2000);
+ await makeRequest(this.address().port, agent);
+ server.close();
+ agent.destroy();
+}));

-Original file line number
+Diff line change
 constkOnKeylog=Symbol('onkeylog');
 constkRequestOptions=Symbol('requestOptions');
 constkRequestAsyncResource=Symbol('requestAsyncResource');
++
 +// TODO(jazelly): make this configurable
 +constHTTP_AGENT_KEEP_ALIVE_TIMEOUT_BUFFER=1000;
 // New Agent code.
 // The largest departure from the previous implementation is that
 socket.unref();
 letagentTimeout=this.options.timeout||0;
 +letcanKeepSocketAlive=true;
 if(socket._httpMessage?.res){
 constkeepAliveHint=socket._httpMessage.res.headers['keep-alive'];
 consthint=/^timeout=(\d+)/.exec(keepAliveHint)?.[1];
 if(hint){
 -constserverHintTimeout=NumberParseInt(hint)*1000;
+-
 -if(serverHintTimeout<agentTimeout){
 +// Let the timer expire before the announced timeout to reduce
 +// the likelihood of ECONNRESET errors
 +letserverHintTimeout=(NumberParseInt(hint)*1000)-HTTP_AGENT_KEEP_ALIVE_TIMEOUT_BUFFER;
 +serverHintTimeout=serverHintTimeout>0 ? serverHintTimeout : 0;
 +if(serverHintTimeout===0){
 +// Cannot safely reuse the socket because the server timeout is
 +// too short
 +canKeepSocketAlive=false;
 +}elseif(serverHintTimeout<agentTimeout){
 agentTimeout=serverHintTimeout;
+}
+}
 socket.setTimeout(agentTimeout);
+}
 -returntrue;
 +returncanKeepSocketAlive;
 };
 Agent.prototype.reuseSocket=functionreuseSocket(socket,req){
-Original file line number
+Diff line change
 constkConnectionsCheckingInterval=Symbol('http.server.connectionsCheckingInterval');
 constHTTP_SERVER_TRACE_EVENT_NAME='http.server.request';
 +// TODO(jazelly): make this configurable
 +constHTTP_SERVER_KEEP_ALIVE_TIMEOUT_BUFFER=1000;
 classHTTPServerAsyncResource{
 constructor(type,socket){
+}
 }elseif(state.outgoing.length===0){
 if(server.keepAliveTimeout&&typeofsocket.setTimeout==='function'){
 -socket.setTimeout(server.keepAliveTimeout);
 +// Increase the internal timeout wrt the advertised value to reduce
 +// the likelihood of ECONNRESET errors.
 +socket.setTimeout(server.keepAliveTimeout+HTTP_SERVER_KEEP_ALIVE_TIMEOUT_BUFFER);
 state.keepAliveTimeoutSet=true;
+}
 }else{
-Original file line number
+Diff line change
@@ @@ -0,0 +1,38 @@ @@
 +'use strict';
++
 +constcommon=require('../common');
 +consthttp=require('http');
++
 +constmakeRequest=(port,agent)=>
 +newPromise((resolve,reject)=>{
 +constreq=http.get(
 +{path: '/', port, agent },
 +(res)=>{
 +res.resume();
 +res.on('end',()=>resolve());
 +},
 +);
 +req.on('error',(e)=>reject(e));
 +req.end();
 +});
++
 +constserver=http.createServer(
 +{keepAliveTimeout: common.platformTimeout(2000),keepAlive: true},
 +common.mustCall((req,res)=>{
 +constbody='hello world\n';
 +res.writeHead(200,{'Content-Length': body.length});
 +res.write(body);
 +res.end();
 +},2)
 +);
++
 +constagent=newhttp.Agent({maxSockets: 5,keepAlive: true});
++
 +server.listen(0,common.mustCall(asyncfunction(){
 +awaitmakeRequest(this.address().port,agent);
 +// Block the event loop for 2 seconds
 +Atomics.wait(newInt32Array(newSharedArrayBuffer(4)),0,0,2000);
 +awaitmakeRequest(this.address().port,agent);
 +server.close();
 +agent.destroy();
 +}));