GeezerVison is showing github.com — http: reduce likelihood of race conditions on keep-alive timeout · nodejs/node@7123bf7
File treeExpand file treeCollapse file tree Open diff view settings Expand file treeCollapse file tree Open diff view settings Collapse file Copy file name to clipboardExpand all lines: lib/_http_agent.js +14-4Lines changed: 14 additions & 4 deletions | Original file line number | Diff line number | Diff line change |
|---|
@@ -49,6 +49,9 @@ const { | 49 | 49 | const kOnKeylog = Symbol('onkeylog'); | 50 | 50 | const kRequestOptions = Symbol('requestOptions'); | 51 | 51 | const kRequestAsyncResource = Symbol('requestAsyncResource'); | | 52 | + | | 53 | +// TODO(jazelly): make this configurable | | 54 | +const HTTP_AGENT_KEEP_ALIVE_TIMEOUT_BUFFER = 1000; | 52 | 55 | // New Agent code. | 53 | 56 |
| 54 | 57 | // The largest departure from the previous implementation is that | @@ -473,6 +476,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) { | 473 | 476 | socket.unref(); | 474 | 477 |
| 475 | 478 | let agentTimeout = this.options.timeout || 0; | | 479 | + let canKeepSocketAlive = true; | 476 | 480 |
| 477 | 481 | if (socket._httpMessage?.res) { | 478 | 482 | const keepAliveHint = socket._httpMessage.res.headers['keep-alive']; | @@ -481,9 +485,15 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) { | 481 | 485 | const hint = /^timeout=(\d+)/.exec(keepAliveHint)?.[1]; | 482 | 486 |
| 483 | 487 | if (hint) { | 484 | | - const serverHintTimeout = NumberParseInt(hint) * 1000; | 485 | | - | 486 | | - if (serverHintTimeout < agentTimeout) { | | 488 | + // Let the timer expire before the announced timeout to reduce | | 489 | + // the likelihood of ECONNRESET errors | | 490 | + let serverHintTimeout = (NumberParseInt(hint) * 1000) - HTTP_AGENT_KEEP_ALIVE_TIMEOUT_BUFFER; | | 491 | + serverHintTimeout = serverHintTimeout > 0 ? serverHintTimeout : 0; | | 492 | + if (serverHintTimeout === 0) { | | 493 | + // Cannot safely reuse the socket because the server timeout is | | 494 | + // too short | | 495 | + canKeepSocketAlive = false; | | 496 | + } else if (serverHintTimeout < agentTimeout) { | 487 | 497 | agentTimeout = serverHintTimeout; | 488 | 498 | } | 489 | 499 | } | @@ -494,7 +504,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) { | 494 | 504 | socket.setTimeout(agentTimeout); | 495 | 505 | } | 496 | 506 |
| 497 | | - return true; | | 507 | + return canKeepSocketAlive; | 498 | 508 | }; | 499 | 509 |
| 500 | 510 | Agent.prototype.reuseSocket = function reuseSocket(socket, req) { | |
Collapse file Copy file name to clipboardExpand all lines: lib/_http_server.js +5-1Lines changed: 5 additions & 1 deletion | Original file line number | Diff line number | Diff line change |
|---|
@@ -182,6 +182,8 @@ const kConnections = Symbol('http.server.connections'); | 182 | 182 | const kConnectionsCheckingInterval = Symbol('http.server.connectionsCheckingInterval'); | 183 | 183 |
| 184 | 184 | const HTTP_SERVER_TRACE_EVENT_NAME = 'http.server.request'; | | 185 | +// TODO(jazelly): make this configurable | | 186 | +const HTTP_SERVER_KEEP_ALIVE_TIMEOUT_BUFFER = 1000; | 185 | 187 |
| 186 | 188 | class HTTPServerAsyncResource { | 187 | 189 | constructor(type, socket) { | @@ -1007,7 +1009,9 @@ function resOnFinish(req, res, socket, state, server) { | 1007 | 1009 | } | 1008 | 1010 | } else if (state.outgoing.length === 0) { | 1009 | 1011 | if (server.keepAliveTimeout && typeof socket.setTimeout === 'function') { | 1010 | | - socket.setTimeout(server.keepAliveTimeout); | | 1012 | + // Increase the internal timeout wrt the advertised value to reduce | | 1013 | + // the likelihood of ECONNRESET errors. | | 1014 | + socket.setTimeout(server.keepAliveTimeout + HTTP_SERVER_KEEP_ALIVE_TIMEOUT_BUFFER); | 1011 | 1015 | state.keepAliveTimeoutSet = true; | 1012 | 1016 | } | 1013 | 1017 | } else { | |
Collapse file Copy file name to clipboard +38Lines changed: 38 additions & 0 deletions | Original file line number | Diff line number | Diff line change |
|---|
| | 1 | +'use strict'; | | 2 | + | | 3 | +const common = require('../common'); | | 4 | +const http = require('http'); | | 5 | + | | 6 | +const makeRequest = (port, agent) => | | 7 | + new Promise((resolve, reject) => { | | 8 | + const req = http.get( | | 9 | + { path: '/', port, agent }, | | 10 | + (res) => { | | 11 | + res.resume(); | | 12 | + res.on('end', () => resolve()); | | 13 | + }, | | 14 | + ); | | 15 | + req.on('error', (e) => reject(e)); | | 16 | + req.end(); | | 17 | + }); | | 18 | + | | 19 | +const server = http.createServer( | | 20 | + { keepAliveTimeout: common.platformTimeout(2000), keepAlive: true }, | | 21 | + common.mustCall((req, res) => { | | 22 | + const body = 'hello world\n'; | | 23 | + res.writeHead(200, { 'Content-Length': body.length }); | | 24 | + res.write(body); | | 25 | + res.end(); | | 26 | + }, 2) | | 27 | +); | | 28 | + | | 29 | +const agent = new http.Agent({ maxSockets: 5, keepAlive: true }); | | 30 | + | | 31 | +server.listen(0, common.mustCall(async function() { | | 32 | + await makeRequest(this.address().port, agent); | | 33 | + // Block the event loop for 2 seconds | | 34 | + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 2000); | | 35 | + await makeRequest(this.address().port, agent); | | 36 | + server.close(); | | 37 | + agent.destroy(); | | 38 | +})); |
|