From 3425286231890d59eb82e877d44ab314f3dce413 Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Wed, 2 Sep 2026 17:23:09 +0100 Subject: [PATCH 01/13] revert keepAliveAgent from prior PR --- lib/addMethod/addMethodREST.js | 6 +----- tests/addMethodREST_test.js | 3 +-- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/addMethod/addMethodREST.js b/lib/addMethod/addMethodREST.js index 9b7d13e..c1475c2 100644 --- a/lib/addMethod/addMethodREST.js +++ b/lib/addMethod/addMethodREST.js @@ -29,15 +29,11 @@ module.exports = function (methodName, config, afterHeadersFunction) { return; } - // TCP keep-alive shorter than 350 sec to prevent AWS NAT idle timeout: https://repost.aws/knowledge-center/lambda-vpc-timeout - const keepAliveAgent = new require('http').Agent({ keepAlive: true, keepAliveMsecs: 349000 }); - // All methods should default to no timeout, maximising the chance // of success. needle.defaults({ open_timeout: 0, - read_timeout: 0, - agent: keepAliveAgent + read_timeout: 0 }); // Validate the input. Errors will be `throw`n if there's anything diff --git a/tests/addMethodREST_test.js b/tests/addMethodREST_test.js index 122c109..e67f245 100644 --- a/tests/addMethodREST_test.js +++ b/tests/addMethodREST_test.js @@ -109,8 +109,7 @@ describe('#addMethodREST', function () { }); after(function (done) { - server.close(); - done(); + server.close(done); }); var threadneedle; From 1174e11bfbd419e266ca4a4cd8cf5f6eb3db291a Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Wed, 2 Sep 2026 17:23:17 +0100 Subject: [PATCH 02/13] Update .eslintrc.js --- .eslintrc.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.eslintrc.js b/.eslintrc.js index 3d8565a..7162332 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -19,6 +19,11 @@ module.exports = { 'no-case-declarations': 0, 'no-inner-declarations': 0, + //`new require('x')` parses as `new require` followed by a plain + //`.x()` call, and throws under module systems whose `require` is not + //constructible (e.g. Jest). + 'no-new-require': 'error', + //global rules 'indent': [ 'error', From 1e04a2751039d168964532e9fa9537eb22fbf2be Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Wed, 2 Sep 2026 17:32:42 +0100 Subject: [PATCH 03/13] new logic to make keepAlive relative to http vs https --- lib/addMethod/addMethodREST.js | 5 ++ lib/addMethod/keepAliveAgent.js | 96 +++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 lib/addMethod/keepAliveAgent.js diff --git a/lib/addMethod/addMethodREST.js b/lib/addMethod/addMethodREST.js index c1475c2..6fac2df 100644 --- a/lib/addMethod/addMethodREST.js +++ b/lib/addMethod/addMethodREST.js @@ -14,6 +14,7 @@ var validateNotExpects = require('./validateNotExpects'); var formatResponse = require('./formatResponse'); var addMethodFunction = require('./addMethodFunction'); +var resolveAgent = require('./keepAliveAgent'); module.exports = function (methodName, config, afterHeadersFunction) { var threadneedle = this; @@ -218,6 +219,10 @@ module.exports = function (methodName, config, afterHeadersFunction) { // console.log(options); logger.info(methodName + ': running ' + method + ' request', request); + // Attach the keep-alive agent, unless the caller supplied their own. + request.options = request.options || {}; + request.options.agent = resolveAgent(request.url, request.options); + // Run a different method for get to not include data switch (method) { case 'get': diff --git a/lib/addMethod/keepAliveAgent.js b/lib/addMethod/keepAliveAgent.js new file mode 100644 index 0000000..0d61fba --- /dev/null +++ b/lib/addMethod/keepAliveAgent.js @@ -0,0 +1,96 @@ +/* +* TCP keep-alive for long-running API calls. +* +* AWS NAT gateways drop connections that sit idle for 350 seconds, so a call +* that waits a long time for its response is killed mid-flight even when there +* is Lambda budget left. The fix is SO_KEEPALIVE on the socket, as AWS +* themselves prescribe: https://repost.aws/knowledge-center/lambda-vpc-timeout +* +* Note this is *not* HTTP keep-alive. Node's `keepAlive: true` means reusing +* connections between requests, and it applies `keepAliveMsecs` only in +* `keepSocketAlive()`, called once a socket is handed back to the pool - after +* the response, and so too late for the socket we care about. These agents +* therefore deliberately do not pool. They exist only to get hold of the socket +* as it is created, leaving socket lifetime as it was before they existed. +*/ +const http = require('http'); +const https = require('https'); + +/* +* Idle time before a keep-alive probe is sent. A probe answered by a healthy +* peer resets the idle timer, so this is also the effective interval between +* probes - TCP_KEEPINTVL only governs retries of an *unanswered* probe. +* +* Sized under the AWS NAT gateway's 350 second idle timeout with 50 seconds of +* margin, so a single probe per idle period is enough to reset the gateway's +* timer. AWS's example uses 1000ms, which probes far more often than is needed +* here for no additional benefit. +*/ +const KEEPALIVE_DELAY = 300000; + +function withKeepAlive (Base) { + return class KeepAliveAgent extends Base { + createConnection (options, callback) { + const socket = super.createConnection(options, callback); + socket.setKeepAlive(true, KEEPALIVE_DELAY); //set SO_KEEPALIVE + return socket; + } + }; +} + +/* +* An agent is bound to one protocol - Node throws ERR_INVALID_PROTOCOL if it is +* handed a request of the other - so we keep a class per protocol and pick by +* the request's URL. +*/ +const agentClasses = { + 'http:': withKeepAlive(http.Agent), + 'https:': withKeepAlive(https.Agent) +}; + +//needle reads a proxy from the options or the environment, upper case then lower +function isProxied (options) { + const envProxy = ( + process.env.HTTP_PROXY || process.env.http_proxy || + process.env.HTTPS_PROXY || process.env.https_proxy + ); + return Boolean(options.proxy || envProxy); +} + +/* +* Resolve the agent for a single request. Returns the caller's own `agent` +* untouched when they set one, so `agent: false` remains a deliberate opt-out. +*/ +module.exports = function resolveAgent (url, options) { + + if (options.agent !== undefined) { + return options.agent; + } + + /* + Behind a proxy needle connects to the proxy, so the socket's protocol is + the proxy's rather than the target's. Rather than duplicate needle's proxy + and NO_PROXY resolution, don't attach an agent at all - keep-alive is an + optimisation, not a guarantee. + */ + if (isProxied(options)) { + return undefined; + } + + //needle prepends `http://` when a URL carries no protocol + const Agent = ( /^https:/i.test(url) ? agentClasses['https:'] : agentClasses['http:'] ); + + /* + Deliberately one agent per request rather than a shared pair. When an agent + is present, needle assigns TLS options (`rejectUnauthorized`, `cert`, `key`, + `pfx`, ...) onto `agent.options` rather than onto the request, so a shared + agent would let one method's TLS settings or client certificate leak into + every later request in the process. Constructing an agent opens no sockets, + and there is no pool to preserve, so per-request costs nothing meaningful. + */ + return new Agent({ keepAlive: false }); + +}; + +module.exports.KEEPALIVE_DELAY = KEEPALIVE_DELAY; +module.exports.agentClasses = agentClasses; From 13fa22b50c601de9092ff2e3d63e6de1188981be Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Wed, 2 Sep 2026 17:33:08 +0100 Subject: [PATCH 04/13] tests --- tests/fixtures/localhost-cert.pem | 21 ++++ tests/fixtures/localhost-key.pem | 28 +++++ tests/keepAliveAgent_test.js | 122 ++++++++++++++++++++ tests/keepAlive_integration_test.js | 165 ++++++++++++++++++++++++++++ 4 files changed, 336 insertions(+) create mode 100644 tests/fixtures/localhost-cert.pem create mode 100644 tests/fixtures/localhost-key.pem create mode 100644 tests/keepAliveAgent_test.js create mode 100644 tests/keepAlive_integration_test.js diff --git a/tests/fixtures/localhost-cert.pem b/tests/fixtures/localhost-cert.pem new file mode 100644 index 0000000..7597f50 --- /dev/null +++ b/tests/fixtures/localhost-cert.pem @@ -0,0 +1,21 @@ +-----BEGIN CERTIFICATE----- +MIIDXzCCAkegAwIBAgIUDOEBXz+s39W3rBQS4kLG/ARfqqcwDQYJKoZIhvcNAQEL +BQAwMTESMBAGA1UEAwwJbG9jYWxob3N0MRswGQYDVQQKDBJ0aHJlYWRuZWVkbGUg +dGVzdHMwHhcNMjYwOTAyMTYyODE1WhcNNDYwODI4MTYyODE1WjAxMRIwEAYDVQQD +DAlsb2NhbGhvc3QxGzAZBgNVBAoMEnRocmVhZG5lZWRsZSB0ZXN0czCCASIwDQYJ +KoZIhvcNAQEBBQADggEPADCCAQoCggEBAJf+FwMIur2P1UAQAEjItL+4NGolAXyi +FOcmqDoC5R3NyYSqGGDZsJpsZDi7YRR+unVHBxkbPPKKxEqeAhlvmRIwnNKoJHjJ +Pcf8JKuxZ4JU4Fea9JDhOXVoZUmo1ARJD8dZErq2iu3bkbbwQ5OwNhJNexHfDdMs +f4n3aMPhb65pwMSRwRK8VlnN9JzXCBqJb06wteuRMXNrVDwMOS16kSnG5XNtq7zW +6FxokAHXlINPVqc4qgvtEvUeCHk/WFJjYc9oZU1dMtuk6AG5YM2lf70llkxl6g+G +SSxD/LySUiYg2N+gg/+9jiXX8aF/AqPLq/wUQuvAMa9zqIvY71oSzHsCAwEAAaNv +MG0wHQYDVR0OBBYEFOly8x/U9QobkV0Eh27TyNikNKBgMB8GA1UdIwQYMBaAFOly +8x/U9QobkV0Eh27TyNikNKBgMA8GA1UdEwEB/wQFMAMBAf8wGgYDVR0RBBMwEYIJ +bG9jYWxob3N0hwR/AAABMA0GCSqGSIb3DQEBCwUAA4IBAQCNbQ/jnuZSjXIWN+v2 +xvBhDWg31/OIFNA0zRj5HH3BDxaudbANOSJgNGlZ5UJrEIGyEczhNA1sK+KkstGA +ugbljElq6YFFehfIb8Fcc0MdW4s8M8mSLIYHFMmxUrNtI796gNkjBaCu6LjG7Jd/ +jX9mqhqmxsI6Z1Uj3PDqeBs8XDKkQMnFpTvyA6kXBwMUFM/PmhLTRBX1xlpUqZv5 +lkgocPZAGXX1Eah/DAFoRawiCs6H1ejvOytCJO9btW7c7Mz2Jy23fvDoqNE/G/gv +5In5KSb2HZtyawh5SsnN+/OTuH3EpQvdcR0xQwLYTJ+EbrqTKOvIN0krFwfqjxnk +aHwC +-----END CERTIFICATE----- diff --git a/tests/fixtures/localhost-key.pem b/tests/fixtures/localhost-key.pem new file mode 100644 index 0000000..12bb3ca --- /dev/null +++ b/tests/fixtures/localhost-key.pem @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCX/hcDCLq9j9VA +EABIyLS/uDRqJQF8ohTnJqg6AuUdzcmEqhhg2bCabGQ4u2EUfrp1RwcZGzzyisRK +ngIZb5kSMJzSqCR4yT3H/CSrsWeCVOBXmvSQ4Tl1aGVJqNQESQ/HWRK6tort25G2 +8EOTsDYSTXsR3w3TLH+J92jD4W+uacDEkcESvFZZzfSc1wgaiW9OsLXrkTFza1Q8 +DDktepEpxuVzbau81uhcaJAB15SDT1anOKoL7RL1Hgh5P1hSY2HPaGVNXTLbpOgB +uWDNpX+9JZZMZeoPhkksQ/y8klImINjfoIP/vY4l1/GhfwKjy6v8FELrwDGvc6iL +2O9aEsx7AgMBAAECggEANAoAif7mpPmGi3EPD9x8Gjow4/i4mhoKaxwOtBICrSIk +sYHlZ9+QukaLR+tL8U70eyvu77cmNmqxi1SvJlNRxusS/oMoPZy1RO/9BDXw2SxD +RWtd+e7LE/pC16XwtWjoeJn0Mi5GweqP6OE5WesWkEyr6vICUz+kiTHG0m4wpTez +g9/isxBLCCYHrQRuCO3UlQphutA9ktsMVFe+1qBZeG2PWCQCFYzHK0snrs1PbSZv +/w8zD36X3b5x93TYPY7ENyV9ue8zL84kXC0H5E8INs9pq1oLeewGqeojIh9lDQEp +Y+t9aVoClGPm3boJ4tH1k7ME9VZvpcmaDZ6Xi0Pc2QKBgQDWrS032s/VdGHi5Jhw +uA7DFuNm2Gmkvo/Cp+PPwY6W0aTYnBuEksZ1v7GvZXYF/VLROueABXQ5GITddbvU +IGyJPDxAtgS1PuqufGiHPrdvbNuKWYAwOy5xioDB6K5OA0zterkcZpG9C5sDmiwR +wmz30gfBv1ytrPOpu41R2uy2eQKBgQC1P/m8Ye0PuzmzXfbUEJq0CNrByLSlQ18Q +9gdej8uNcsFm2Be7F54WZIhuiHvi0ab2tVxWfsWp198XVZp2mMgfiK9r7qo9fmj6 +xgdYyYWDAQeczi5OVDKYxl7b4R0E4PeXQIuAb/xW1XKoSjRUVAPAsIIRAf/76lgo +NtlQCvjtkwKBgFohLPnlURrCGRLEfMfeTrxTkLeuJnR3WS4VhMzF69KgRAB5UghQ +AyiOidAk3e9X0vxrKaSTJZ+PDsFX27sMveTEOFvGz6U0vBzzuIMHrsYGQwoL14jo +X/BlgPdodD3mntaZjrxAx/FBvRw/Dz+JjGxjbsRGTmfQVCCv0H5MVtOpAoGBALOS +OLj9RENLuTUOKVedQ8iO5T0MvnzlrLA/MLntOTxgr2BXQ9um4IdK/yiTrDnigMr4 +kA1Z+Df3mh2iQDCz2cH0R+hlQuE99oBN5kV/Evnh8UrXs2UDYkWec6jg9UE6KdL+ +rbeIO7dELh6xtfq+aiFkPtje5GEPolvlS5RT6qBlAoGALy76FkmqIC4ZQwUJi3vM +2ak89NxDI0dQnEglPPtcJxs/6yRLUxsdc2O+ZaUetG85KY6UFFD8VluyMiFg7NYJ +1PTTW+vDE61HN04dI8tPgpwMUOYkdF/rLOE+QKq2ruM75zDxvc0O7vCxex3oSFHr +GrULG57pJuWWF18VnCEviYc= +-----END PRIVATE KEY----- diff --git a/tests/keepAliveAgent_test.js b/tests/keepAliveAgent_test.js new file mode 100644 index 0000000..f53c2ac --- /dev/null +++ b/tests/keepAliveAgent_test.js @@ -0,0 +1,122 @@ +var assert = require('assert'); +var http = require('http'); +var https = require('https'); + +var resolveAgent = require('../lib/addMethod/keepAliveAgent'); + + +describe('#keepAliveAgent', function () { + + describe('Protocol selection', function () { + + /* + `https.Agent` extends `http.Agent`, so `instanceof` cannot tell them + apart. `protocol` is the property Node itself compares against the + request, and mismatching it is what threw ERR_INVALID_PROTOCOL. + */ + + it('should return an https agent for an https url', function () { + assert.strictEqual(resolveAgent('https://example.com/thing', {}).protocol, 'https:'); + }); + + it('should return an http agent for an http url', function () { + assert.strictEqual(resolveAgent('http://example.com/thing', {}).protocol, 'http:'); + }); + + it('should ignore the case of the protocol', function () { + assert.strictEqual(resolveAgent('HTTPS://example.com', {}).protocol, 'https:'); + }); + + it('should default to http when the url carries no protocol, as needle does', function () { + assert.strictEqual(resolveAgent('example.com/thing', {}).protocol, 'http:'); + }); + + it('should not be fooled by `https` appearing later in the url', function () { + assert.strictEqual(resolveAgent('http://example.com/?next=https://x.com', {}).protocol, 'http:'); + }); + + }); + + describe('Caller overrides', function () { + + it('should return a caller supplied agent untouched', function () { + var mine = new https.Agent({ keepAlive: true }); + assert.strictEqual(resolveAgent('https://example.com', { agent: mine }), mine); + }); + + it('should pass `false` through, so opting out remains possible', function () { + assert.strictEqual(resolveAgent('https://example.com', { agent: false }), false); + }); + + it('should attach an agent when none was supplied', function () { + assert.ok(resolveAgent('https://example.com', {}) instanceof http.Agent); + }); + + }); + + describe('Proxies', function () { + + /* + Behind a proxy needle connects to the proxy, so the socket's protocol + is the proxy's, not the target's. Attaching an agent picked from the + target url would reintroduce the protocol mismatch. + */ + + it('should not attach an agent when `options.proxy` is set', function () { + assert.strictEqual(resolveAgent('https://example.com', { proxy: 'http://proxy:8080' }), undefined); + }); + + [ 'HTTP_PROXY', 'http_proxy', 'HTTPS_PROXY', 'https_proxy' ].forEach(function (name) { + + it('should not attach an agent when ' + name + ' is set', function () { + process.env[name] = 'http://proxy:8080'; + try { + assert.strictEqual(resolveAgent('https://example.com', {}), undefined); + } finally { + delete process.env[name]; + } + }); + + }); + + }); + + describe('Agent configuration', function () { + + it('should not pool connections, leaving socket lifetime unchanged', function () { + assert.strictEqual(resolveAgent('https://example.com', {}).keepAlive, false); + }); + + it('should arm keep-alive with usable margin inside the NAT idle timeout', function () { + //AWS NAT gateways drop connections that sit idle for 350 seconds + var NAT_IDLE_TIMEOUT = 350000; + var REQUIRED_MARGIN = 30000; + + assert.ok( + resolveAgent.KEEPALIVE_DELAY <= NAT_IDLE_TIMEOUT - REQUIRED_MARGIN, + 'a probe must land early enough to reset the gateway timer, not just before it' + ); + }); + + it('should return a new agent per request', function () { + var first = resolveAgent('https://example.com', {}); + var second = resolveAgent('https://example.com', {}); + assert.notStrictEqual(first, second); + }); + + /* + needle assigns TLS options onto `agent.options` when an agent is + present, so a shared agent would leak one method's TLS settings or + client certificate into every later request in the process. + */ + it('should not share TLS options between requests', function () { + var first = resolveAgent('https://example.com', {}); + first.options.rejectUnauthorized = false; + + var second = resolveAgent('https://example.com', {}); + assert.notStrictEqual(second.options.rejectUnauthorized, false); + }); + + }); + +}); diff --git a/tests/keepAlive_integration_test.js b/tests/keepAlive_integration_test.js new file mode 100644 index 0000000..f07aece --- /dev/null +++ b/tests/keepAlive_integration_test.js @@ -0,0 +1,165 @@ +var assert = require('assert'); +var fs = require('fs'); +var path = require('path'); +var https = require('https'); +var net = require('net'); + +var { randString } = require('../lib/utils/mout'); +var ThreadNeedle = require('../'); + + +describe('#keepAlive integration', function () { + + var server; + var host; + + //Records every setKeepAlive call made while these tests run + var armed; + var originalSetKeepAlive; + + before(function (done) { + originalSetKeepAlive = net.Socket.prototype.setKeepAlive; + net.Socket.prototype.setKeepAlive = function (enable, delay) { + armed.push({ enable: enable, delay: delay, at: Date.now() }); + return originalSetKeepAlive.apply(this, arguments); + }; + + server = https.createServer({ + key: fs.readFileSync(path.join(__dirname, 'fixtures/localhost-key.pem')), + cert: fs.readFileSync(path.join(__dirname, 'fixtures/localhost-cert.pem')) + }); + + server.listen(0, function () { + host = 'https://localhost:' + server.address().port; + done(); + }); + }); + + after(function (done) { + net.Socket.prototype.setKeepAlive = originalSetKeepAlive; + server.close(done); + }); + + beforeEach(function () { + armed = []; + }); + + /* + The whole REST suite otherwise runs against `http://localhost`, which is + why an http-only agent installed as a needle default went unnoticed. An + https request is the case that threw ERR_INVALID_PROTOCOL. + */ + it('should complete an https request', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + server.once('request', function (req, res) { + res.writeHead(200, { 'content-type': 'application/json' }); + res.end(JSON.stringify({ ok: true })); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + options: { rejectUnauthorized: false } + }); + + threadneedle[name]({}).done(function (result) { + assert.deepStrictEqual(result.body, { ok: true }); + done(); + }, done); + }); + + /* + The point of the fix. Node's own `keepAliveMsecs` is applied when a socket + returns to the pool, i.e. after the response - too late for a call that + idles behind the NAT gateway while awaiting a slow reply. + */ + it('should arm keep-alive before the response arrives', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + var armedBeforeServerReplied; + + server.once('request', function (req, res) { + armedBeforeServerReplied = armed.length > 0; + //Hold the request open, as a slow API would + setTimeout(function () { + res.writeHead(200); + res.end('ok'); + }, 150); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + options: { rejectUnauthorized: false } + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual( + armedBeforeServerReplied, true, + 'keep-alive should be armed while the request is still in flight' + ); + assert.strictEqual(armed[0].enable, true); + assert.strictEqual(armed[0].delay, require('../lib/addMethod/keepAliveAgent').KEEPALIVE_DELAY); + done(); + }, done); + }); + + it('should use an agent supplied by the method instead of its own', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + var used = false; + + class RecordingAgent extends https.Agent { + createConnection (options, callback) { + used = true; + return super.createConnection(options, callback); + } + } + var mine = new RecordingAgent({ keepAlive: false, rejectUnauthorized: false }); + + server.once('request', function (req, res) { + res.writeHead(200); + res.end('ok'); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + //A function value survives substitution, which an agent object does not + options: { rejectUnauthorized: false, agent: function () { return mine; } } + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(used, true, 'the method\'s own agent should have been used'); + done(); + }, done); + }); + + it('should let a method opt out of keep-alive with `agent: false`', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + server.once('request', function (req, res) { + res.writeHead(200); + res.end('ok'); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + options: { rejectUnauthorized: false, agent: function () { return false; } } + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(armed.length, 0, 'no keep-alive should have been armed'); + done(); + }, done); + }); + +}); From 4a345dd938b4c95355a6017abcd191704d054701 Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Wed, 2 Sep 2026 17:33:47 +0100 Subject: [PATCH 05/13] patch bump --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index f9e8182..d53c447 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@trayio/threadneedle", - "version": "1.19.0-beta.0", + "version": "1.19.1", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@trayio/threadneedle", - "version": "1.19.0-beta.0", + "version": "1.19.1", "license": "MIT", "dependencies": { "@trayio/needle": "^4.0.0", diff --git a/package.json b/package.json index 6566e36..8e4acbf 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@trayio/threadneedle", - "version": "1.19.0", + "version": "1.19.1", "description": "A framework for simplifying working with HTTP-based APIs.", "main": "lib/index.js", "directories": { From 926804a4204d51b79a1f870fe085f615e3f95679 Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Thu, 3 Sep 2026 09:59:54 +0100 Subject: [PATCH 06/13] test update --- tests/fixtures/localhost-cert.pem | 21 --- tests/fixtures/localhost-key.pem | 28 --- tests/keepAlive_integration_test.js | 257 ++++++++++++++++------------ 3 files changed, 150 insertions(+), 156 deletions(-) delete mode 100644 tests/fixtures/localhost-cert.pem delete mode 100644 tests/fixtures/localhost-key.pem diff --git a/tests/fixtures/localhost-cert.pem b/tests/fixtures/localhost-cert.pem deleted file mode 100644 index 7597f50..0000000 --- a/tests/fixtures/localhost-cert.pem +++ /dev/null @@ -1,21 +0,0 @@ ------BEGIN CERTIFICATE----- -MIIDXzCCAkegAwIBAgIUDOEBXz+s39W3rBQS4kLG/ARfqqcwDQYJKoZIhvcNAQEL -BQAwMTESMBAGA1UEAwwJbG9jYWxob3N0MRswGQYDVQQKDBJ0aHJlYWRuZWVkbGUg -dGVzdHMwHhcNMjYwOTAyMTYyODE1WhcNNDYwODI4MTYyODE1WjAxMRIwEAYDVQQD -DAlsb2NhbGhvc3QxGzAZBgNVBAoMEnRocmVhZG5lZWRsZSB0ZXN0czCCASIwDQYJ -KoZIhvcNAQEBBQADggEPADCCAQoCggEBAJf+FwMIur2P1UAQAEjItL+4NGolAXyi -FOcmqDoC5R3NyYSqGGDZsJpsZDi7YRR+unVHBxkbPPKKxEqeAhlvmRIwnNKoJHjJ -Pcf8JKuxZ4JU4Fea9JDhOXVoZUmo1ARJD8dZErq2iu3bkbbwQ5OwNhJNexHfDdMs -f4n3aMPhb65pwMSRwRK8VlnN9JzXCBqJb06wteuRMXNrVDwMOS16kSnG5XNtq7zW -6FxokAHXlINPVqc4qgvtEvUeCHk/WFJjYc9oZU1dMtuk6AG5YM2lf70llkxl6g+G -SSxD/LySUiYg2N+gg/+9jiXX8aF/AqPLq/wUQuvAMa9zqIvY71oSzHsCAwEAAaNv -MG0wHQYDVR0OBBYEFOly8x/U9QobkV0Eh27TyNikNKBgMB8GA1UdIwQYMBaAFOly -8x/U9QobkV0Eh27TyNikNKBgMA8GA1UdEwEB/wQFMAMBAf8wGgYDVR0RBBMwEYIJ -bG9jYWxob3N0hwR/AAABMA0GCSqGSIb3DQEBCwUAA4IBAQCNbQ/jnuZSjXIWN+v2 -xvBhDWg31/OIFNA0zRj5HH3BDxaudbANOSJgNGlZ5UJrEIGyEczhNA1sK+KkstGA -ugbljElq6YFFehfIb8Fcc0MdW4s8M8mSLIYHFMmxUrNtI796gNkjBaCu6LjG7Jd/ -jX9mqhqmxsI6Z1Uj3PDqeBs8XDKkQMnFpTvyA6kXBwMUFM/PmhLTRBX1xlpUqZv5 -lkgocPZAGXX1Eah/DAFoRawiCs6H1ejvOytCJO9btW7c7Mz2Jy23fvDoqNE/G/gv -5In5KSb2HZtyawh5SsnN+/OTuH3EpQvdcR0xQwLYTJ+EbrqTKOvIN0krFwfqjxnk -aHwC ------END CERTIFICATE----- diff --git a/tests/fixtures/localhost-key.pem b/tests/fixtures/localhost-key.pem deleted file mode 100644 index 12bb3ca..0000000 --- a/tests/fixtures/localhost-key.pem +++ /dev/null @@ -1,28 +0,0 @@ ------BEGIN PRIVATE KEY----- -MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCX/hcDCLq9j9VA -EABIyLS/uDRqJQF8ohTnJqg6AuUdzcmEqhhg2bCabGQ4u2EUfrp1RwcZGzzyisRK -ngIZb5kSMJzSqCR4yT3H/CSrsWeCVOBXmvSQ4Tl1aGVJqNQESQ/HWRK6tort25G2 -8EOTsDYSTXsR3w3TLH+J92jD4W+uacDEkcESvFZZzfSc1wgaiW9OsLXrkTFza1Q8 -DDktepEpxuVzbau81uhcaJAB15SDT1anOKoL7RL1Hgh5P1hSY2HPaGVNXTLbpOgB -uWDNpX+9JZZMZeoPhkksQ/y8klImINjfoIP/vY4l1/GhfwKjy6v8FELrwDGvc6iL -2O9aEsx7AgMBAAECggEANAoAif7mpPmGi3EPD9x8Gjow4/i4mhoKaxwOtBICrSIk -sYHlZ9+QukaLR+tL8U70eyvu77cmNmqxi1SvJlNRxusS/oMoPZy1RO/9BDXw2SxD -RWtd+e7LE/pC16XwtWjoeJn0Mi5GweqP6OE5WesWkEyr6vICUz+kiTHG0m4wpTez -g9/isxBLCCYHrQRuCO3UlQphutA9ktsMVFe+1qBZeG2PWCQCFYzHK0snrs1PbSZv -/w8zD36X3b5x93TYPY7ENyV9ue8zL84kXC0H5E8INs9pq1oLeewGqeojIh9lDQEp -Y+t9aVoClGPm3boJ4tH1k7ME9VZvpcmaDZ6Xi0Pc2QKBgQDWrS032s/VdGHi5Jhw -uA7DFuNm2Gmkvo/Cp+PPwY6W0aTYnBuEksZ1v7GvZXYF/VLROueABXQ5GITddbvU -IGyJPDxAtgS1PuqufGiHPrdvbNuKWYAwOy5xioDB6K5OA0zterkcZpG9C5sDmiwR -wmz30gfBv1ytrPOpu41R2uy2eQKBgQC1P/m8Ye0PuzmzXfbUEJq0CNrByLSlQ18Q -9gdej8uNcsFm2Be7F54WZIhuiHvi0ab2tVxWfsWp198XVZp2mMgfiK9r7qo9fmj6 -xgdYyYWDAQeczi5OVDKYxl7b4R0E4PeXQIuAb/xW1XKoSjRUVAPAsIIRAf/76lgo -NtlQCvjtkwKBgFohLPnlURrCGRLEfMfeTrxTkLeuJnR3WS4VhMzF69KgRAB5UghQ -AyiOidAk3e9X0vxrKaSTJZ+PDsFX27sMveTEOFvGz6U0vBzzuIMHrsYGQwoL14jo -X/BlgPdodD3mntaZjrxAx/FBvRw/Dz+JjGxjbsRGTmfQVCCv0H5MVtOpAoGBALOS -OLj9RENLuTUOKVedQ8iO5T0MvnzlrLA/MLntOTxgr2BXQ9um4IdK/yiTrDnigMr4 -kA1Z+Df3mh2iQDCz2cH0R+hlQuE99oBN5kV/Evnh8UrXs2UDYkWec6jg9UE6KdL+ -rbeIO7dELh6xtfq+aiFkPtje5GEPolvlS5RT6qBlAoGALy76FkmqIC4ZQwUJi3vM -2ak89NxDI0dQnEglPPtcJxs/6yRLUxsdc2O+ZaUetG85KY6UFFD8VluyMiFg7NYJ -1PTTW+vDE61HN04dI8tPgpwMUOYkdF/rLOE+QKq2ruM75zDxvc0O7vCxex3oSFHr -GrULG57pJuWWF18VnCEviYc= ------END PRIVATE KEY----- diff --git a/tests/keepAlive_integration_test.js b/tests/keepAlive_integration_test.js index f07aece..9b04de6 100644 --- a/tests/keepAlive_integration_test.js +++ b/tests/keepAlive_integration_test.js @@ -1,10 +1,9 @@ var assert = require('assert'); -var fs = require('fs'); -var path = require('path'); -var https = require('https'); +var http = require('http'); var net = require('net'); var { randString } = require('../lib/utils/mout'); +var resolveAgent = require('../lib/addMethod/keepAliveAgent'); var ThreadNeedle = require('../'); @@ -12,6 +11,7 @@ describe('#keepAlive integration', function () { var server; var host; + var closedPort; //Records every setKeepAlive call made while these tests run var armed; @@ -24,14 +24,21 @@ describe('#keepAlive integration', function () { return originalSetKeepAlive.apply(this, arguments); }; - server = https.createServer({ - key: fs.readFileSync(path.join(__dirname, 'fixtures/localhost-key.pem')), - cert: fs.readFileSync(path.join(__dirname, 'fixtures/localhost-cert.pem')) - }); + server = http.createServer(); server.listen(0, function () { - host = 'https://localhost:' + server.address().port; - done(); + host = 'http://localhost:' + server.address().port; + + /* + Reserve a port and immediately release it, so requests to it are + refused rather than answered. Used to check protocol handling + without needing a TLS server, and so without a checked-in key. + */ + var scout = net.createServer(); + scout.listen(0, function () { + closedPort = scout.address().port; + scout.close(done); + }); }); }); @@ -44,122 +51,158 @@ describe('#keepAlive integration', function () { armed = []; }); - /* - The whole REST suite otherwise runs against `http://localhost`, which is - why an http-only agent installed as a needle default went unnoticed. An - https request is the case that threw ERR_INVALID_PROTOCOL. - */ - it('should complete an https request', function (done) { - var name = randString(10); - var threadneedle = new ThreadNeedle(); - - server.once('request', function (req, res) { - res.writeHead(200, { 'content-type': 'application/json' }); - res.end(JSON.stringify({ ok: true })); + function errorCodeOf (error) { + return ( ( error.body && ( error.body.code || error.body.errno ) ) || error.code ); + } + + describe('Protocol handling', function () { + + /* + The whole REST suite otherwise runs against http, which is why an + http-only agent installed as a needle default went unnoticed. Node + compares the agent's protocol against the request before it opens a + socket, so reaching the network at all is the thing worth asserting - + no TLS server, and therefore no key material, is needed to prove it. + */ + it('should reach the network on an https request rather than reject the agent', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + threadneedle.addMethod(name, { + method: 'get', + url: 'https://127.0.0.1:' + closedPort + '/' + name + }); + + threadneedle[name]({}).done(function () { + done(new Error('nothing should be listening on this port')); + }, function (error) { + assert.strictEqual(errorCodeOf(error), 'ECONNREFUSED'); + done(); + }); }); - threadneedle.addMethod(name, { - method: 'get', - url: host + '/' + name, - expects: 200, - options: { rejectUnauthorized: false } + /* + The failure the test above guards against, pinned deliberately: if Node + ever stops rejecting a mismatched agent, that test would silently lose + its teeth, and this one would start failing to say so. + */ + it('should show that a mismatched agent is what breaks such a request', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + threadneedle.addMethod(name, { + method: 'get', + url: 'https://127.0.0.1:' + closedPort + '/' + name, + //An http agent on an https url, as 1.19.0 installed globally + options: { agent: function () { return new http.Agent({ keepAlive: false }); } } + }); + + threadneedle[name]({}).done(function () { + done(new Error('a mismatched agent should not have connected')); + }, function (error) { + assert.strictEqual(errorCodeOf(error), 'ERR_INVALID_PROTOCOL'); + done(); + }); }); - threadneedle[name]({}).done(function (result) { - assert.deepStrictEqual(result.body, { ok: true }); - done(); - }, done); }); - /* - The point of the fix. Node's own `keepAliveMsecs` is applied when a socket - returns to the pool, i.e. after the response - too late for a call that - idles behind the NAT gateway while awaiting a slow reply. - */ - it('should arm keep-alive before the response arrives', function (done) { - var name = randString(10); - var threadneedle = new ThreadNeedle(); - var armedBeforeServerReplied; - - server.once('request', function (req, res) { - armedBeforeServerReplied = armed.length > 0; - //Hold the request open, as a slow API would - setTimeout(function () { - res.writeHead(200); - res.end('ok'); - }, 150); - }); - - threadneedle.addMethod(name, { - method: 'get', - url: host + '/' + name, - expects: 200, - options: { rejectUnauthorized: false } + describe('Arming', function () { + + /* + The point of the fix. Node's own `keepAliveMsecs` is applied when a + socket returns to the pool, i.e. after the response - too late for a + call that idles behind the NAT gateway while awaiting a slow reply. + */ + it('should arm keep-alive before the response arrives', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + var armedBeforeServerReplied; + + server.once('request', function (req, res) { + armedBeforeServerReplied = armed.length > 0; + //Hold the request open, as a slow API would + setTimeout(function () { + res.writeHead(200); + res.end('ok'); + }, 150); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200 + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual( + armedBeforeServerReplied, true, + 'keep-alive should be armed while the request is still in flight' + ); + assert.strictEqual(armed[0].enable, true); + assert.strictEqual(armed[0].delay, resolveAgent.KEEPALIVE_DELAY); + done(); + }, done); }); - threadneedle[name]({}).done(function () { - assert.strictEqual( - armedBeforeServerReplied, true, - 'keep-alive should be armed while the request is still in flight' - ); - assert.strictEqual(armed[0].enable, true); - assert.strictEqual(armed[0].delay, require('../lib/addMethod/keepAliveAgent').KEEPALIVE_DELAY); - done(); - }, done); }); - it('should use an agent supplied by the method instead of its own', function (done) { - var name = randString(10); - var threadneedle = new ThreadNeedle(); - var used = false; + describe('Caller overrides', function () { - class RecordingAgent extends https.Agent { - createConnection (options, callback) { - used = true; - return super.createConnection(options, callback); - } - } - var mine = new RecordingAgent({ keepAlive: false, rejectUnauthorized: false }); + it('should use an agent supplied by the method instead of its own', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + var used = false; - server.once('request', function (req, res) { - res.writeHead(200); - res.end('ok'); - }); + class RecordingAgent extends http.Agent { + createConnection (options, callback) { + used = true; + return super.createConnection(options, callback); + } + } + var mine = new RecordingAgent({ keepAlive: false }); - threadneedle.addMethod(name, { - method: 'get', - url: host + '/' + name, - expects: 200, - //A function value survives substitution, which an agent object does not - options: { rejectUnauthorized: false, agent: function () { return mine; } } + server.once('request', function (req, res) { + res.writeHead(200); + res.end('ok'); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + //A function value survives substitution, which an agent object does not + options: { agent: function () { return mine; } } + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(used, true, 'the method\'s own agent should have been used'); + done(); + }, done); }); - threadneedle[name]({}).done(function () { - assert.strictEqual(used, true, 'the method\'s own agent should have been used'); - done(); - }, done); - }); - - it('should let a method opt out of keep-alive with `agent: false`', function (done) { - var name = randString(10); - var threadneedle = new ThreadNeedle(); + it('should let a method opt out of keep-alive with `agent: false`', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); - server.once('request', function (req, res) { - res.writeHead(200); - res.end('ok'); - }); - - threadneedle.addMethod(name, { - method: 'get', - url: host + '/' + name, - expects: 200, - options: { rejectUnauthorized: false, agent: function () { return false; } } + server.once('request', function (req, res) { + res.writeHead(200); + res.end('ok'); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + options: { agent: function () { return false; } } + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(armed.length, 0, 'no keep-alive should have been armed'); + done(); + }, done); }); - threadneedle[name]({}).done(function () { - assert.strictEqual(armed.length, 0, 'no keep-alive should have been armed'); - done(); - }, done); }); }); From e61527f826b7eecf41a12b39c0795ef930911e32 Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Thu, 3 Sep 2026 10:18:00 +0100 Subject: [PATCH 07/13] review feedback fix --- lib/addMethod/addMethodREST.js | 2 +- lib/addMethod/keepAliveAgent.js | 101 +++++++++++++------------- tests/keepAliveAgent_test.js | 105 +++++++++++++--------------- tests/keepAlive_integration_test.js | 69 ++++++++++++++++++ 4 files changed, 172 insertions(+), 105 deletions(-) diff --git a/lib/addMethod/addMethodREST.js b/lib/addMethod/addMethodREST.js index 6fac2df..72b4c2a 100644 --- a/lib/addMethod/addMethodREST.js +++ b/lib/addMethod/addMethodREST.js @@ -221,7 +221,7 @@ module.exports = function (methodName, config, afterHeadersFunction) { // Attach the keep-alive agent, unless the caller supplied their own. request.options = request.options || {}; - request.options.agent = resolveAgent(request.url, request.options); + request.options.agent = resolveAgent(request.options); // Run a different method for get to not include data switch (method) { diff --git a/lib/addMethod/keepAliveAgent.js b/lib/addMethod/keepAliveAgent.js index 0d61fba..c38139e 100644 --- a/lib/addMethod/keepAliveAgent.js +++ b/lib/addMethod/keepAliveAgent.js @@ -9,9 +9,9 @@ * Note this is *not* HTTP keep-alive. Node's `keepAlive: true` means reusing * connections between requests, and it applies `keepAliveMsecs` only in * `keepSocketAlive()`, called once a socket is handed back to the pool - after -* the response, and so too late for the socket we care about. These agents -* therefore deliberately do not pool. They exist only to get hold of the socket -* as it is created, leaving socket lifetime as it was before they existed. +* the response, and so too late for the socket we care about. This agent +* therefore deliberately does not pool. It exists only to get hold of the socket +* as it is created, leaving socket lifetime as it was before it existed. */ const http = require('http'); const https = require('https'); @@ -28,69 +28,74 @@ const https = require('https'); */ const KEEPALIVE_DELAY = 300000; -function withKeepAlive (Base) { - return class KeepAliveAgent extends Base { - createConnection (options, callback) { - const socket = super.createConnection(options, callback); - socket.setKeepAlive(true, KEEPALIVE_DELAY); //set SO_KEEPALIVE - return socket; - } - }; -} - /* -* An agent is bound to one protocol - Node throws ERR_INVALID_PROTOCOL if it is -* handed a request of the other - so we keep a class per protocol and pick by -* the request's URL. +* Connection factories, one per protocol. These are real agents, used purely +* for their `createConnection`, so that TLS setup - servername, ALPN, session +* resumption - remains Node's job rather than something reimplemented here. */ -const agentClasses = { - 'http:': withKeepAlive(http.Agent), - 'https:': withKeepAlive(https.Agent) +const factories = { + 'http:': new http.Agent({ keepAlive: false }), + 'https:': new https.Agent({ keepAlive: false }) }; -//needle reads a proxy from the options or the environment, upper case then lower -function isProxied (options) { - const envProxy = ( - process.env.HTTP_PROXY || process.env.http_proxy || - process.env.HTTPS_PROXY || process.env.https_proxy - ); - return Boolean(options.proxy || envProxy); +class KeepAliveAgent extends http.Agent { + + constructor (options) { + super(options); + + /* + Node throws ERR_INVALID_PROTOCOL when an agent declares a protocol + that differs from the request's, and only performs that check when the + agent declares one at all. This agent serves either protocol, choosing + per connection below, so it declares none. + + This matters for redirects: needle follows them by re-entering + `send_request` with the same config object, so the agent chosen for the + first request is reused for the redirect target. An agent fixed to one + protocol would throw - uncaught, from inside needle's response handler + - as soon as a request crossed from http to https or back. + */ + this.protocol = undefined; + } + + createConnection (options, callback) { + /* + `options.protocol` is set per request by needle, from the proxy's url + when proxying and the target's otherwise, so it describes the socket + actually being opened rather than where the request started. + */ + const factory = ( factories[options.protocol] || factories['http:'] ); + + const socket = factory.createConnection(options, callback); + socket.setKeepAlive(true, KEEPALIVE_DELAY); //set SO_KEEPALIVE + + return socket; + } + } /* * Resolve the agent for a single request. Returns the caller's own `agent` * untouched when they set one, so `agent: false` remains a deliberate opt-out. */ -module.exports = function resolveAgent (url, options) { +module.exports = function resolveAgent (options) { if (options.agent !== undefined) { return options.agent; } /* - Behind a proxy needle connects to the proxy, so the socket's protocol is - the proxy's rather than the target's. Rather than duplicate needle's proxy - and NO_PROXY resolution, don't attach an agent at all - keep-alive is an - optimisation, not a guarantee. - */ - if (isProxied(options)) { - return undefined; - } - - //needle prepends `http://` when a URL carries no protocol - const Agent = ( /^https:/i.test(url) ? agentClasses['https:'] : agentClasses['http:'] ); - - /* - Deliberately one agent per request rather than a shared pair. When an agent - is present, needle assigns TLS options (`rejectUnauthorized`, `cert`, `key`, - `pfx`, ...) onto `agent.options` rather than onto the request, so a shared - agent would let one method's TLS settings or client certificate leak into - every later request in the process. Constructing an agent opens no sockets, - and there is no pool to preserve, so per-request costs nothing meaningful. + Deliberately one agent per request rather than a shared instance. When an + agent is present, needle assigns TLS options (`rejectUnauthorized`, `cert`, + `key`, `pfx`, ...) onto `agent.options` rather than onto the request, so a + shared agent would let one method's TLS settings or client certificate leak + into every later request in the process. Constructing an agent opens no + sockets, and there is no pool to preserve, so per-request costs nothing. */ - return new Agent({ keepAlive: false }); + return new KeepAliveAgent({ keepAlive: false }); }; module.exports.KEEPALIVE_DELAY = KEEPALIVE_DELAY; -module.exports.agentClasses = agentClasses; +module.exports.KeepAliveAgent = KeepAliveAgent; +module.exports.factories = factories; diff --git a/tests/keepAliveAgent_test.js b/tests/keepAliveAgent_test.js index f53c2ac..8061fea 100644 --- a/tests/keepAliveAgent_test.js +++ b/tests/keepAliveAgent_test.js @@ -1,82 +1,78 @@ var assert = require('assert'); var http = require('http'); var https = require('https'); +var net = require('net'); var resolveAgent = require('../lib/addMethod/keepAliveAgent'); describe('#keepAliveAgent', function () { - describe('Protocol selection', function () { - - /* - `https.Agent` extends `http.Agent`, so `instanceof` cannot tell them - apart. `protocol` is the property Node itself compares against the - request, and mismatching it is what threw ERR_INVALID_PROTOCOL. - */ - - it('should return an https agent for an https url', function () { - assert.strictEqual(resolveAgent('https://example.com/thing', {}).protocol, 'https:'); - }); - - it('should return an http agent for an http url', function () { - assert.strictEqual(resolveAgent('http://example.com/thing', {}).protocol, 'http:'); - }); - - it('should ignore the case of the protocol', function () { - assert.strictEqual(resolveAgent('HTTPS://example.com', {}).protocol, 'https:'); - }); - - it('should default to http when the url carries no protocol, as needle does', function () { - assert.strictEqual(resolveAgent('example.com/thing', {}).protocol, 'http:'); - }); - - it('should not be fooled by `https` appearing later in the url', function () { - assert.strictEqual(resolveAgent('http://example.com/?next=https://x.com', {}).protocol, 'http:'); - }); - - }); - describe('Caller overrides', function () { it('should return a caller supplied agent untouched', function () { var mine = new https.Agent({ keepAlive: true }); - assert.strictEqual(resolveAgent('https://example.com', { agent: mine }), mine); + assert.strictEqual(resolveAgent({ agent: mine }), mine); }); it('should pass `false` through, so opting out remains possible', function () { - assert.strictEqual(resolveAgent('https://example.com', { agent: false }), false); + assert.strictEqual(resolveAgent({ agent: false }), false); }); it('should attach an agent when none was supplied', function () { - assert.ok(resolveAgent('https://example.com', {}) instanceof http.Agent); + assert.ok(resolveAgent({}) instanceof http.Agent); }); }); - describe('Proxies', function () { + describe('Protocol handling', function () { /* - Behind a proxy needle connects to the proxy, so the socket's protocol - is the proxy's, not the target's. Attaching an agent picked from the - target url would reintroduce the protocol mismatch. - */ + Node only enforces its agent/request protocol check when the agent + declares a protocol. Declaring none is what lets one agent serve both, + which in turn is what stops a redirect from http to https - where + needle reuses the original agent - throwing ERR_INVALID_PROTOCOL. - it('should not attach an agent when `options.proxy` is set', function () { - assert.strictEqual(resolveAgent('https://example.com', { proxy: 'http://proxy:8080' }), undefined); + If a Node upgrade ever changes that, this is the test that says so. + */ + it('should declare no protocol, so either is accepted', function () { + assert.strictEqual(resolveAgent({}).protocol, undefined); }); - [ 'HTTP_PROXY', 'http_proxy', 'HTTPS_PROXY', 'https_proxy' ].forEach(function (name) { - - it('should not attach an agent when ' + name + ' is set', function () { - process.env[name] = 'http://proxy:8080'; - try { - assert.strictEqual(resolveAgent('https://example.com', {}), undefined); - } finally { - delete process.env[name]; - } + function connectionFor (protocol) { + var agent = resolveAgent({}); + var chosen = null; + + var originals = {}; + Object.keys(resolveAgent.factories).forEach(function (key) { + originals[key] = resolveAgent.factories[key].createConnection; + resolveAgent.factories[key].createConnection = function () { + chosen = key; + return new net.Socket(); + }; }); + try { + agent.createConnection({ protocol: protocol, host: 'example.com', port: 443 }, function () {}); + } finally { + Object.keys(originals).forEach(function (key) { + resolveAgent.factories[key].createConnection = originals[key]; + }); + } + + return chosen; + } + + it('should open a tls connection for an https request', function () { + assert.strictEqual(connectionFor('https:'), 'https:'); + }); + + it('should open a plain connection for an http request', function () { + assert.strictEqual(connectionFor('http:'), 'http:'); + }); + + it('should fall back to a plain connection when the protocol is unknown', function () { + assert.strictEqual(connectionFor(undefined), 'http:'); }); }); @@ -84,7 +80,7 @@ describe('#keepAliveAgent', function () { describe('Agent configuration', function () { it('should not pool connections, leaving socket lifetime unchanged', function () { - assert.strictEqual(resolveAgent('https://example.com', {}).keepAlive, false); + assert.strictEqual(resolveAgent({}).keepAlive, false); }); it('should arm keep-alive with usable margin inside the NAT idle timeout', function () { @@ -99,9 +95,7 @@ describe('#keepAliveAgent', function () { }); it('should return a new agent per request', function () { - var first = resolveAgent('https://example.com', {}); - var second = resolveAgent('https://example.com', {}); - assert.notStrictEqual(first, second); + assert.notStrictEqual(resolveAgent({}), resolveAgent({})); }); /* @@ -110,11 +104,10 @@ describe('#keepAliveAgent', function () { client certificate into every later request in the process. */ it('should not share TLS options between requests', function () { - var first = resolveAgent('https://example.com', {}); + var first = resolveAgent({}); first.options.rejectUnauthorized = false; - var second = resolveAgent('https://example.com', {}); - assert.notStrictEqual(second.options.rejectUnauthorized, false); + assert.notStrictEqual(resolveAgent({}).options.rejectUnauthorized, false); }); }); diff --git a/tests/keepAlive_integration_test.js b/tests/keepAlive_integration_test.js index 9b04de6..f035beb 100644 --- a/tests/keepAlive_integration_test.js +++ b/tests/keepAlive_integration_test.js @@ -86,6 +86,75 @@ describe('#keepAlive integration', function () { ever stops rejecting a mismatched agent, that test would silently lose its teeth, and this one would start failing to say so. */ + /* + needle follows redirects by re-entering `send_request` with the same + config object, so the agent attached to the first request is reused for + the redirect target. An agent fixed to one protocol threw + ERR_INVALID_PROTOCOL here - uncaught, from inside needle's response + handler, taking the process down with it. + + Connectors opt into this: http-client and http-client-vpc expose + redirect following as a user option, and several others set + `follow_max` directly. + */ + it('should follow a redirect that switches protocol', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + server.once('request', function (req, res) { + res.writeHead(302, { location: 'https://127.0.0.1:' + closedPort + '/moved' }); + res.end(); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + options: { follow_max: 3 } + }); + + threadneedle[name]({}).done(function () { + done(new Error('nothing should be listening on the redirect target')); + }, function (error) { + /* + Reaching the redirect target at all is the point: the protocol + switch no longer throws, so the request fails on the refused + connection instead. + */ + assert.strictEqual(errorCodeOf(error), 'ECONNREFUSED'); + done(); + }); + }); + + /* + Behind a proxy, needle connects to the proxy rather than the target, so + the socket's protocol is the proxy's. Choosing the connection type from + `options.protocol` handles that without the agent needing to know a + proxy is involved. + */ + it('should connect over the proxy\'s protocol, not the target\'s', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + + //The plain http server above stands in for an http proxy + server.once('request', function (req, res) { + res.writeHead(200, { 'content-type': 'text/plain' }); + res.end('via proxy'); + }); + + threadneedle.addMethod(name, { + method: 'get', + url: 'https://example.com/' + name, + expects: 200, + options: { proxy: host } + }); + + threadneedle[name]({}).done(function (result) { + assert.strictEqual(result.body, 'via proxy'); + assert.strictEqual(armed[0].delay, resolveAgent.KEEPALIVE_DELAY); + done(); + }, done); + }); + it('should show that a mismatched agent is what breaks such a request', function (done) { var name = randString(10); var threadneedle = new ThreadNeedle(); From 28463648c96343616a90fcd229053c55a0338cea Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Thu, 3 Sep 2026 10:20:01 +0100 Subject: [PATCH 08/13] rename refactor --- lib/addMethod/addMethodREST.js | 4 ++-- lib/addMethod/keepAliveAgent.js | 2 +- tests/keepAliveAgent_test.js | 30 ++++++++++++++--------------- tests/keepAlive_integration_test.js | 6 +++--- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/lib/addMethod/addMethodREST.js b/lib/addMethod/addMethodREST.js index 72b4c2a..109c21e 100644 --- a/lib/addMethod/addMethodREST.js +++ b/lib/addMethod/addMethodREST.js @@ -14,7 +14,7 @@ var validateNotExpects = require('./validateNotExpects'); var formatResponse = require('./formatResponse'); var addMethodFunction = require('./addMethodFunction'); -var resolveAgent = require('./keepAliveAgent'); +var resolveKeepAliveAgent = require('./keepAliveAgent'); module.exports = function (methodName, config, afterHeadersFunction) { var threadneedle = this; @@ -221,7 +221,7 @@ module.exports = function (methodName, config, afterHeadersFunction) { // Attach the keep-alive agent, unless the caller supplied their own. request.options = request.options || {}; - request.options.agent = resolveAgent(request.options); + request.options.agent = resolveKeepAliveAgent(request.options); // Run a different method for get to not include data switch (method) { diff --git a/lib/addMethod/keepAliveAgent.js b/lib/addMethod/keepAliveAgent.js index c38139e..98a4599 100644 --- a/lib/addMethod/keepAliveAgent.js +++ b/lib/addMethod/keepAliveAgent.js @@ -78,7 +78,7 @@ class KeepAliveAgent extends http.Agent { * Resolve the agent for a single request. Returns the caller's own `agent` * untouched when they set one, so `agent: false` remains a deliberate opt-out. */ -module.exports = function resolveAgent (options) { +module.exports = function resolveKeepAliveAgent (options) { if (options.agent !== undefined) { return options.agent; diff --git a/tests/keepAliveAgent_test.js b/tests/keepAliveAgent_test.js index 8061fea..46d5450 100644 --- a/tests/keepAliveAgent_test.js +++ b/tests/keepAliveAgent_test.js @@ -3,7 +3,7 @@ var http = require('http'); var https = require('https'); var net = require('net'); -var resolveAgent = require('../lib/addMethod/keepAliveAgent'); +var resolveKeepAliveAgent = require('../lib/addMethod/keepAliveAgent'); describe('#keepAliveAgent', function () { @@ -12,15 +12,15 @@ describe('#keepAliveAgent', function () { it('should return a caller supplied agent untouched', function () { var mine = new https.Agent({ keepAlive: true }); - assert.strictEqual(resolveAgent({ agent: mine }), mine); + assert.strictEqual(resolveKeepAliveAgent({ agent: mine }), mine); }); it('should pass `false` through, so opting out remains possible', function () { - assert.strictEqual(resolveAgent({ agent: false }), false); + assert.strictEqual(resolveKeepAliveAgent({ agent: false }), false); }); it('should attach an agent when none was supplied', function () { - assert.ok(resolveAgent({}) instanceof http.Agent); + assert.ok(resolveKeepAliveAgent({}) instanceof http.Agent); }); }); @@ -36,17 +36,17 @@ describe('#keepAliveAgent', function () { If a Node upgrade ever changes that, this is the test that says so. */ it('should declare no protocol, so either is accepted', function () { - assert.strictEqual(resolveAgent({}).protocol, undefined); + assert.strictEqual(resolveKeepAliveAgent({}).protocol, undefined); }); function connectionFor (protocol) { - var agent = resolveAgent({}); + var agent = resolveKeepAliveAgent({}); var chosen = null; var originals = {}; - Object.keys(resolveAgent.factories).forEach(function (key) { - originals[key] = resolveAgent.factories[key].createConnection; - resolveAgent.factories[key].createConnection = function () { + Object.keys(resolveKeepAliveAgent.factories).forEach(function (key) { + originals[key] = resolveKeepAliveAgent.factories[key].createConnection; + resolveKeepAliveAgent.factories[key].createConnection = function () { chosen = key; return new net.Socket(); }; @@ -56,7 +56,7 @@ describe('#keepAliveAgent', function () { agent.createConnection({ protocol: protocol, host: 'example.com', port: 443 }, function () {}); } finally { Object.keys(originals).forEach(function (key) { - resolveAgent.factories[key].createConnection = originals[key]; + resolveKeepAliveAgent.factories[key].createConnection = originals[key]; }); } @@ -80,7 +80,7 @@ describe('#keepAliveAgent', function () { describe('Agent configuration', function () { it('should not pool connections, leaving socket lifetime unchanged', function () { - assert.strictEqual(resolveAgent({}).keepAlive, false); + assert.strictEqual(resolveKeepAliveAgent({}).keepAlive, false); }); it('should arm keep-alive with usable margin inside the NAT idle timeout', function () { @@ -89,13 +89,13 @@ describe('#keepAliveAgent', function () { var REQUIRED_MARGIN = 30000; assert.ok( - resolveAgent.KEEPALIVE_DELAY <= NAT_IDLE_TIMEOUT - REQUIRED_MARGIN, + resolveKeepAliveAgent.KEEPALIVE_DELAY <= NAT_IDLE_TIMEOUT - REQUIRED_MARGIN, 'a probe must land early enough to reset the gateway timer, not just before it' ); }); it('should return a new agent per request', function () { - assert.notStrictEqual(resolveAgent({}), resolveAgent({})); + assert.notStrictEqual(resolveKeepAliveAgent({}), resolveKeepAliveAgent({})); }); /* @@ -104,10 +104,10 @@ describe('#keepAliveAgent', function () { client certificate into every later request in the process. */ it('should not share TLS options between requests', function () { - var first = resolveAgent({}); + var first = resolveKeepAliveAgent({}); first.options.rejectUnauthorized = false; - assert.notStrictEqual(resolveAgent({}).options.rejectUnauthorized, false); + assert.notStrictEqual(resolveKeepAliveAgent({}).options.rejectUnauthorized, false); }); }); diff --git a/tests/keepAlive_integration_test.js b/tests/keepAlive_integration_test.js index f035beb..6364e95 100644 --- a/tests/keepAlive_integration_test.js +++ b/tests/keepAlive_integration_test.js @@ -3,7 +3,7 @@ var http = require('http'); var net = require('net'); var { randString } = require('../lib/utils/mout'); -var resolveAgent = require('../lib/addMethod/keepAliveAgent'); +var resolveKeepAliveAgent = require('../lib/addMethod/keepAliveAgent'); var ThreadNeedle = require('../'); @@ -150,7 +150,7 @@ describe('#keepAlive integration', function () { threadneedle[name]({}).done(function (result) { assert.strictEqual(result.body, 'via proxy'); - assert.strictEqual(armed[0].delay, resolveAgent.KEEPALIVE_DELAY); + assert.strictEqual(armed[0].delay, resolveKeepAliveAgent.KEEPALIVE_DELAY); done(); }, done); }); @@ -209,7 +209,7 @@ describe('#keepAlive integration', function () { 'keep-alive should be armed while the request is still in flight' ); assert.strictEqual(armed[0].enable, true); - assert.strictEqual(armed[0].delay, resolveAgent.KEEPALIVE_DELAY); + assert.strictEqual(armed[0].delay, resolveKeepAliveAgent.KEEPALIVE_DELAY); done(); }, done); }); From f3652a40aeb146fff2b53b689307c8e19eea9f06 Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Thu, 3 Sep 2026 12:52:29 +0100 Subject: [PATCH 09/13] added `disableKeepAliveAgent ` flag to disable new agent logic --- lib/addMethod/addMethodREST.js | 5 +- .../globalize/disableKeepAliveAgent.js | 31 ++++++++ lib/addMethod/globalize/index.js | 3 +- lib/addMethod/keepAliveAgent.js | 13 +++- tests/globalize_test.js | 73 +++++++++++++++++++ tests/keepAliveAgent_test.js | 21 ++++++ tests/keepAlive_integration_test.js | 72 +++++++++++++++++- 7 files changed, 214 insertions(+), 4 deletions(-) create mode 100644 lib/addMethod/globalize/disableKeepAliveAgent.js diff --git a/lib/addMethod/addMethodREST.js b/lib/addMethod/addMethodREST.js index 109c21e..cf0aca8 100644 --- a/lib/addMethod/addMethodREST.js +++ b/lib/addMethod/addMethodREST.js @@ -221,7 +221,10 @@ module.exports = function (methodName, config, afterHeadersFunction) { // Attach the keep-alive agent, unless the caller supplied their own. request.options = request.options || {}; - request.options.agent = resolveKeepAliveAgent(request.options); + request.options.agent = resolveKeepAliveAgent( + request.options, + globalize.disableKeepAliveAgent.call(threadneedle, config) + ); // Run a different method for get to not include data switch (method) { diff --git a/lib/addMethod/globalize/disableKeepAliveAgent.js b/lib/addMethod/globalize/disableKeepAliveAgent.js new file mode 100644 index 0000000..a0de0cc --- /dev/null +++ b/lib/addMethod/globalize/disableKeepAliveAgent.js @@ -0,0 +1,31 @@ +/* +* Whether to skip attaching the TCP keep-alive agent to a request. +* +* The agent is on by default, since the AWS NAT gateway idle timeout it works +* around applies to every outbound call. A method may turn it off with +* `disableKeepAliveAgent: true`, and a connector may turn it off for all of its +* methods by setting the same key in its globals. A method can also turn it +* back on with `disableKeepAliveAgent: false` where its connector has disabled +* it wholesale. +* +* Note this does not consult `localOnly`, so unlike every other global a +* method's `globals: false` does not discard it. That is intentional: the flag +* gets set because keep-alive breaks a service, and `globals: false` is common +* on auth endpoints for unrelated reasons (global headers, baseUrl). Letting it +* silently re-enable the agent on those endpoints would defeat the point. +*/ +const _ = require('lodash'); + +module.exports = function (config) { + + if (_.isBoolean(config.disableKeepAliveAgent)) { + return config.disableKeepAliveAgent; + } + + if (_.isBoolean(this._globalOptions.disableKeepAliveAgent)) { + return this._globalOptions.disableKeepAliveAgent; + } + + return false; + +}; diff --git a/lib/addMethod/globalize/index.js b/lib/addMethod/globalize/index.js index fd1e37a..496d898 100644 --- a/lib/addMethod/globalize/index.js +++ b/lib/addMethod/globalize/index.js @@ -9,5 +9,6 @@ module.exports = { notExpects: require('./notExpects'), afterSuccess: require('./afterSuccess'), afterHeaders: require('./afterHeaders'), - afterFailure: require('./afterFailure') + afterFailure: require('./afterFailure'), + disableKeepAliveAgent: require('./disableKeepAliveAgent') }; diff --git a/lib/addMethod/keepAliveAgent.js b/lib/addMethod/keepAliveAgent.js index 98a4599..dbf6a91 100644 --- a/lib/addMethod/keepAliveAgent.js +++ b/lib/addMethod/keepAliveAgent.js @@ -12,10 +12,15 @@ * the response, and so too late for the socket we care about. This agent * therefore deliberately does not pool. It exists only to get hold of the socket * as it is created, leaving socket lifetime as it was before it existed. +* +* Can be turned off per method or per connector with +* `disableKeepAliveAgent: true` - see globalize/disableKeepAliveAgent.js. */ const http = require('http'); const https = require('https'); +const logger = require('../logger'); + /* * Idle time before a keep-alive probe is sent. A probe answered by a healthy * peer resets the idle timer, so this is also the effective interval between @@ -78,12 +83,18 @@ class KeepAliveAgent extends http.Agent { * Resolve the agent for a single request. Returns the caller's own `agent` * untouched when they set one, so `agent: false` remains a deliberate opt-out. */ -module.exports = function resolveKeepAliveAgent (options) { +module.exports = function resolveKeepAliveAgent (options, disabled) { if (options.agent !== undefined) { return options.agent; } + //`disableKeepAliveAgent`, set on the method or the connector's globals + if (disabled === true) { + logger.info('keep-alive agent disabled by configuration; not attaching one'); + return undefined; + } + /* Deliberately one agent per request rather than a shared instance. When an agent is present, needle assigns TLS options (`rejectUnauthorized`, `cert`, diff --git a/tests/globalize_test.js b/tests/globalize_test.js index 50c3259..6b06702 100644 --- a/tests/globalize_test.js +++ b/tests/globalize_test.js @@ -2067,4 +2067,77 @@ describe('#globalize', function () { }); + describe('#disableKeepAliveAgent', function () { + + it('should be enabled by default', function () { + const sample = { _globalOptions: {} }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, {}), + false + ); + }); + + it('should allow a method to disable it', function () { + const sample = { _globalOptions: {} }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { disableKeepAliveAgent: true }), + true + ); + }); + + it('should allow a connector to disable it for every method', function () { + const sample = { _globalOptions: { disableKeepAliveAgent: true } }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, {}), + true + ); + }); + + it('should let a method re-enable it where the connector disabled it', function () { + const sample = { _globalOptions: { disableKeepAliveAgent: true } }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { disableKeepAliveAgent: false }), + false + ); + }); + + it('should ignore non-boolean values', function () { + const sample = { _globalOptions: {} }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { disableKeepAliveAgent: 'true' }), + false + ); + }); + + /* + Deliberately unlike every other global: a kill switch set because + keep-alive breaks a service must not be undone by `globals: false`, + which is common on auth endpoints for unrelated reasons. + */ + it('should not be discarded by `globals: false`', function () { + const sample = { _globalOptions: { disableKeepAliveAgent: true } }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { globals: false }), + true + ); + }); + + it('should not be discarded by a per-key global opt-out', function () { + const sample = { _globalOptions: { disableKeepAliveAgent: true } }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { globals: { disableKeepAliveAgent: false } }), + true + ); + }); + + }); + + }); diff --git a/tests/keepAliveAgent_test.js b/tests/keepAliveAgent_test.js index 46d5450..16d621c 100644 --- a/tests/keepAliveAgent_test.js +++ b/tests/keepAliveAgent_test.js @@ -25,6 +25,27 @@ describe('#keepAliveAgent', function () { }); + describe('Disabling', function () { + + it('should not attach an agent when disabled', function () { + assert.strictEqual(resolveKeepAliveAgent({}, true), undefined); + }); + + it('should attach an agent when explicitly not disabled', function () { + assert.ok(resolveKeepAliveAgent({}, false) instanceof http.Agent); + }); + + it('should attach an agent when nothing was said either way', function () { + assert.ok(resolveKeepAliveAgent({}, undefined) instanceof http.Agent); + }); + + it('should still honour a caller supplied agent when disabled', function () { + var mine = new https.Agent({ keepAlive: true }); + assert.strictEqual(resolveKeepAliveAgent({ agent: mine }, true), mine); + }); + + }); + describe('Protocol handling', function () { /* diff --git a/tests/keepAlive_integration_test.js b/tests/keepAlive_integration_test.js index 6364e95..2f94594 100644 --- a/tests/keepAlive_integration_test.js +++ b/tests/keepAlive_integration_test.js @@ -216,6 +216,72 @@ describe('#keepAlive integration', function () { }); + describe('Disabling', function () { + + function respondOk () { + server.once('request', function (req, res) { + res.writeHead(200); + res.end('ok'); + }); + } + + it('should attach no agent when a method disables it', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + respondOk(); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + disableKeepAliveAgent: true + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(armed.length, 0, 'no keep-alive should have been armed'); + done(); + }, done); + }); + + it('should attach no agent when the connector disables it globally', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + threadneedle.global({ disableKeepAliveAgent: true }); + respondOk(); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200 + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(armed.length, 0, 'no keep-alive should have been armed'); + done(); + }, done); + }); + + it('should let a method re-enable it where the connector disabled it', function (done) { + var name = randString(10); + var threadneedle = new ThreadNeedle(); + threadneedle.global({ disableKeepAliveAgent: true }); + respondOk(); + + threadneedle.addMethod(name, { + method: 'get', + url: host + '/' + name, + expects: 200, + disableKeepAliveAgent: false + }); + + threadneedle[name]({}).done(function () { + assert.strictEqual(armed[0].delay, resolveKeepAliveAgent.KEEPALIVE_DELAY); + done(); + }, done); + }); + + }); + describe('Caller overrides', function () { it('should use an agent supplied by the method instead of its own', function (done) { @@ -240,7 +306,11 @@ describe('#keepAlive integration', function () { method: 'get', url: host + '/' + name, expects: 200, - //A function value survives substitution, which an agent object does not + /* + Substitution invokes function values and keeps what they return, so + an agent returned this way keeps its prototype. Passing the agent + itself would have it walked as a plain object instead. + */ options: { agent: function () { return mine; } } }); From e5c71dec15b98ffd98be11592e914357eb4f3c56 Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Thu, 3 Sep 2026 13:02:53 +0100 Subject: [PATCH 10/13] simplify comments --- .../globalize/disableKeepAliveAgent.js | 20 ++---- lib/addMethod/keepAliveAgent.js | 72 ++++++------------- tests/globalize_test.js | 6 +- tests/keepAliveAgent_test.js | 15 ++-- tests/keepAlive_integration_test.js | 57 +++++---------- 5 files changed, 50 insertions(+), 120 deletions(-) diff --git a/lib/addMethod/globalize/disableKeepAliveAgent.js b/lib/addMethod/globalize/disableKeepAliveAgent.js index a0de0cc..057aede 100644 --- a/lib/addMethod/globalize/disableKeepAliveAgent.js +++ b/lib/addMethod/globalize/disableKeepAliveAgent.js @@ -1,18 +1,12 @@ /* -* Whether to skip attaching the TCP keep-alive agent to a request. +* Whether to skip the keep-alive agent. On by default; a method or a connector's +* globals can set `disableKeepAliveAgent: true`, and a method can set it back to +* `false` where its connector disabled it wholesale. * -* The agent is on by default, since the AWS NAT gateway idle timeout it works -* around applies to every outbound call. A method may turn it off with -* `disableKeepAliveAgent: true`, and a connector may turn it off for all of its -* methods by setting the same key in its globals. A method can also turn it -* back on with `disableKeepAliveAgent: false` where its connector has disabled -* it wholesale. -* -* Note this does not consult `localOnly`, so unlike every other global a -* method's `globals: false` does not discard it. That is intentional: the flag -* gets set because keep-alive breaks a service, and `globals: false` is common -* on auth endpoints for unrelated reasons (global headers, baseUrl). Letting it -* silently re-enable the agent on those endpoints would defeat the point. +* Unlike other globals, `globals: false` does not discard this. The flag gets +* set because keep-alive breaks a service, and `globals: false` is common on +* auth endpoints for unrelated reasons - silently re-enabling the agent there +* would defeat the point. */ const _ = require('lodash'); diff --git a/lib/addMethod/keepAliveAgent.js b/lib/addMethod/keepAliveAgent.js index dbf6a91..57293c6 100644 --- a/lib/addMethod/keepAliveAgent.js +++ b/lib/addMethod/keepAliveAgent.js @@ -1,20 +1,14 @@ /* -* TCP keep-alive for long-running API calls. +* TCP keep-alive, so a call still waiting on a slow API is not dropped by the +* AWS NAT gateway's 350 second idle timeout. +* See https://repost.aws/knowledge-center/lambda-vpc-timeout * -* AWS NAT gateways drop connections that sit idle for 350 seconds, so a call -* that waits a long time for its response is killed mid-flight even when there -* is Lambda budget left. The fix is SO_KEEPALIVE on the socket, as AWS -* themselves prescribe: https://repost.aws/knowledge-center/lambda-vpc-timeout +* Not HTTP keep-alive: Node's `keepAlive: true` pools connections between +* requests and only arms the socket once it returns to the pool, after the +* response - too late. So this agent does not pool. It exists only to arm the +* socket as it is created. * -* Note this is *not* HTTP keep-alive. Node's `keepAlive: true` means reusing -* connections between requests, and it applies `keepAliveMsecs` only in -* `keepSocketAlive()`, called once a socket is handed back to the pool - after -* the response, and so too late for the socket we care about. This agent -* therefore deliberately does not pool. It exists only to get hold of the socket -* as it is created, leaving socket lifetime as it was before it existed. -* -* Can be turned off per method or per connector with -* `disableKeepAliveAgent: true` - see globalize/disableKeepAliveAgent.js. +* Switch off with `disableKeepAliveAgent` - see globalize/disableKeepAliveAgent.js */ const http = require('http'); const https = require('https'); @@ -22,22 +16,13 @@ const https = require('https'); const logger = require('../logger'); /* -* Idle time before a keep-alive probe is sent. A probe answered by a healthy -* peer resets the idle timer, so this is also the effective interval between -* probes - TCP_KEEPINTVL only governs retries of an *unanswered* probe. -* -* Sized under the AWS NAT gateway's 350 second idle timeout with 50 seconds of -* margin, so a single probe per idle period is enough to reset the gateway's -* timer. AWS's example uses 1000ms, which probes far more often than is needed -* here for no additional benefit. +* Idle time before the first probe, and the gap between probes thereafter, since +* an answered probe resets the timer. Under the gateway's 350 seconds with +* margin, so one probe per idle period is enough. */ const KEEPALIVE_DELAY = 300000; -/* -* Connection factories, one per protocol. These are real agents, used purely -* for their `createConnection`, so that TLS setup - servername, ALPN, session -* resumption - remains Node's job rather than something reimplemented here. -*/ +//Real agents, used only for their `createConnection`, so TLS setup stays Node's job const factories = { 'http:': new http.Agent({ keepAlive: false }), 'https:': new https.Agent({ keepAlive: false }) @@ -49,26 +34,15 @@ class KeepAliveAgent extends http.Agent { super(options); /* - Node throws ERR_INVALID_PROTOCOL when an agent declares a protocol - that differs from the request's, and only performs that check when the - agent declares one at all. This agent serves either protocol, choosing - per connection below, so it declares none. - - This matters for redirects: needle follows them by re-entering - `send_request` with the same config object, so the agent chosen for the - first request is reused for the redirect target. An agent fixed to one - protocol would throw - uncaught, from inside needle's response handler - - as soon as a request crossed from http to https or back. + Node only rejects a protocol mismatch when the agent declares one, and + this agent serves both. Declaring none is also what keeps redirects + working, since needle reuses the same agent for the redirect target. */ this.protocol = undefined; } createConnection (options, callback) { - /* - `options.protocol` is set per request by needle, from the proxy's url - when proxying and the target's otherwise, so it describes the socket - actually being opened rather than where the request started. - */ + //needle sets this per request: the proxy's protocol when proxying, else the target's const factory = ( factories[options.protocol] || factories['http:'] ); const socket = factory.createConnection(options, callback); @@ -79,10 +53,7 @@ class KeepAliveAgent extends http.Agent { } -/* -* Resolve the agent for a single request. Returns the caller's own `agent` -* untouched when they set one, so `agent: false` remains a deliberate opt-out. -*/ +//A caller's own `agent` is returned untouched, so `agent: false` stays an opt-out module.exports = function resolveKeepAliveAgent (options, disabled) { if (options.agent !== undefined) { @@ -96,12 +67,9 @@ module.exports = function resolveKeepAliveAgent (options, disabled) { } /* - Deliberately one agent per request rather than a shared instance. When an - agent is present, needle assigns TLS options (`rejectUnauthorized`, `cert`, - `key`, `pfx`, ...) onto `agent.options` rather than onto the request, so a - shared agent would let one method's TLS settings or client certificate leak - into every later request in the process. Constructing an agent opens no - sockets, and there is no pool to preserve, so per-request costs nothing. + One per request, not shared: needle writes TLS options (`rejectUnauthorized`, + `cert`, `key`, ...) onto `agent.options`, so sharing would leak one method's + TLS settings into later requests. Agents open no sockets, so this is free. */ return new KeepAliveAgent({ keepAlive: false }); diff --git a/tests/globalize_test.js b/tests/globalize_test.js index 6b06702..29541dd 100644 --- a/tests/globalize_test.js +++ b/tests/globalize_test.js @@ -2114,11 +2114,7 @@ describe('#globalize', function () { ); }); - /* - Deliberately unlike every other global: a kill switch set because - keep-alive breaks a service must not be undone by `globals: false`, - which is common on auth endpoints for unrelated reasons. - */ + //Unlike other globals - a switch set because keep-alive breaks a service it('should not be discarded by `globals: false`', function () { const sample = { _globalOptions: { disableKeepAliveAgent: true } }; diff --git a/tests/keepAliveAgent_test.js b/tests/keepAliveAgent_test.js index 16d621c..4f3810e 100644 --- a/tests/keepAliveAgent_test.js +++ b/tests/keepAliveAgent_test.js @@ -49,12 +49,9 @@ describe('#keepAliveAgent', function () { describe('Protocol handling', function () { /* - Node only enforces its agent/request protocol check when the agent - declares a protocol. Declaring none is what lets one agent serve both, - which in turn is what stops a redirect from http to https - where - needle reuses the original agent - throwing ERR_INVALID_PROTOCOL. - - If a Node upgrade ever changes that, this is the test that says so. + Declaring no protocol is what lets one agent serve both, and so what + keeps cross-protocol redirects working. If a Node upgrade changes that + check, this test says so. */ it('should declare no protocol, so either is accepted', function () { assert.strictEqual(resolveKeepAliveAgent({}).protocol, undefined); @@ -119,11 +116,7 @@ describe('#keepAliveAgent', function () { assert.notStrictEqual(resolveKeepAliveAgent({}), resolveKeepAliveAgent({})); }); - /* - needle assigns TLS options onto `agent.options` when an agent is - present, so a shared agent would leak one method's TLS settings or - client certificate into every later request in the process. - */ + //needle writes TLS options onto `agent.options`, so sharing would leak them it('should not share TLS options between requests', function () { var first = resolveKeepAliveAgent({}); first.options.rejectUnauthorized = false; diff --git a/tests/keepAlive_integration_test.js b/tests/keepAlive_integration_test.js index 2f94594..d7a1e62 100644 --- a/tests/keepAlive_integration_test.js +++ b/tests/keepAlive_integration_test.js @@ -30,9 +30,8 @@ describe('#keepAlive integration', function () { host = 'http://localhost:' + server.address().port; /* - Reserve a port and immediately release it, so requests to it are - refused rather than answered. Used to check protocol handling - without needing a TLS server, and so without a checked-in key. + A port reserved then released, so requests to it are refused. Lets us + check protocol handling without a TLS server, and so without a key. */ var scout = net.createServer(); scout.listen(0, function () { @@ -58,11 +57,9 @@ describe('#keepAlive integration', function () { describe('Protocol handling', function () { /* - The whole REST suite otherwise runs against http, which is why an - http-only agent installed as a needle default went unnoticed. Node - compares the agent's protocol against the request before it opens a - socket, so reaching the network at all is the thing worth asserting - - no TLS server, and therefore no key material, is needed to prove it. + The rest of the suite runs against http, which is how an http-only agent + went unnoticed. Node checks the agent's protocol before opening a socket, + so reaching the network at all is what proves this. */ it('should reach the network on an https request rather than reject the agent', function (done) { var name = randString(10); @@ -82,20 +79,9 @@ describe('#keepAlive integration', function () { }); /* - The failure the test above guards against, pinned deliberately: if Node - ever stops rejecting a mismatched agent, that test would silently lose - its teeth, and this one would start failing to say so. - */ - /* - needle follows redirects by re-entering `send_request` with the same - config object, so the agent attached to the first request is reused for - the redirect target. An agent fixed to one protocol threw - ERR_INVALID_PROTOCOL here - uncaught, from inside needle's response - handler, taking the process down with it. - - Connectors opt into this: http-client and http-client-vpc expose - redirect following as a user option, and several others set - `follow_max` directly. + needle reuses the same agent for the redirect target, so an agent fixed + to one protocol threw here - uncaught, taking the process down. Several + connectors follow redirects, and http-client exposes it as a user option. */ it('should follow a redirect that switches protocol', function (done) { var name = randString(10); @@ -115,21 +101,15 @@ describe('#keepAlive integration', function () { threadneedle[name]({}).done(function () { done(new Error('nothing should be listening on the redirect target')); }, function (error) { - /* - Reaching the redirect target at all is the point: the protocol - switch no longer throws, so the request fails on the refused - connection instead. - */ + //Reaching the target at all is the point - it fails on the refused connection assert.strictEqual(errorCodeOf(error), 'ECONNREFUSED'); done(); }); }); /* - Behind a proxy, needle connects to the proxy rather than the target, so - the socket's protocol is the proxy's. Choosing the connection type from - `options.protocol` handles that without the agent needing to know a - proxy is involved. + needle connects to the proxy, not the target, so the socket's protocol is + the proxy's. Picking from `options.protocol` handles that for free. */ it('should connect over the proxy\'s protocol, not the target\'s', function (done) { var name = randString(10); @@ -155,6 +135,10 @@ describe('#keepAlive integration', function () { }, done); }); + /* + Pins the failure the tests above rely on: if Node stopped rejecting a + mismatched agent, they would quietly lose their teeth. + */ it('should show that a mismatched agent is what breaks such a request', function (done) { var name = randString(10); var threadneedle = new ThreadNeedle(); @@ -179,9 +163,8 @@ describe('#keepAlive integration', function () { describe('Arming', function () { /* - The point of the fix. Node's own `keepAliveMsecs` is applied when a - socket returns to the pool, i.e. after the response - too late for a - call that idles behind the NAT gateway while awaiting a slow reply. + The point of the fix: Node's own `keepAliveMsecs` is applied only once a + socket returns to the pool, after the response, which is too late. */ it('should arm keep-alive before the response arrives', function (done) { var name = randString(10); @@ -306,11 +289,7 @@ describe('#keepAlive integration', function () { method: 'get', url: host + '/' + name, expects: 200, - /* - Substitution invokes function values and keeps what they return, so - an agent returned this way keeps its prototype. Passing the agent - itself would have it walked as a plain object instead. - */ + //Substitution keeps what a function returns, so the agent's prototype survives options: { agent: function () { return mine; } } }); From c1a54ac19057ba803c1357d10b1299c430f86303 Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Thu, 3 Sep 2026 13:44:50 +0100 Subject: [PATCH 11/13] change explicit method flag behaviour --- .../globalize/disableKeepAliveAgent.js | 17 +++++++++++++---- tests/globalize_test.js | 12 +++++++++++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/lib/addMethod/globalize/disableKeepAliveAgent.js b/lib/addMethod/globalize/disableKeepAliveAgent.js index 057aede..3056d64 100644 --- a/lib/addMethod/globalize/disableKeepAliveAgent.js +++ b/lib/addMethod/globalize/disableKeepAliveAgent.js @@ -3,10 +3,14 @@ * globals can set `disableKeepAliveAgent: true`, and a method can set it back to * `false` where its connector disabled it wholesale. * -* Unlike other globals, `globals: false` does not discard this. The flag gets -* set because keep-alive breaks a service, and `globals: false` is common on -* auth endpoints for unrelated reasons - silently re-enabling the agent there -* would defeat the point. +* A blanket `globals: false` does not discard this, unlike other globals. The +* flag gets set because keep-alive breaks a service, and `globals: false` is +* common on auth endpoints for unrelated reasons - silently re-enabling the +* agent there would defeat the point. Naming the key does discard it, since +* that is a deliberate statement about this flag rather than collateral. +* +* Hence checking `globals` directly rather than through `localOnly`, which +* cannot tell the blanket opt-out from the per-key one. */ const _ = require('lodash'); @@ -16,6 +20,11 @@ module.exports = function (config) { return config.disableKeepAliveAgent; } + //`globals: { disableKeepAliveAgent: false }` - drop the global, leaving the default + if (_.get(config, 'globals.disableKeepAliveAgent') === false) { + return false; + } + if (_.isBoolean(this._globalOptions.disableKeepAliveAgent)) { return this._globalOptions.disableKeepAliveAgent; } diff --git a/tests/globalize_test.js b/tests/globalize_test.js index 29541dd..90d3708 100644 --- a/tests/globalize_test.js +++ b/tests/globalize_test.js @@ -2124,11 +2124,21 @@ describe('#globalize', function () { ); }); - it('should not be discarded by a per-key global opt-out', function () { + //Naming the key is deliberate, unlike a blanket `globals: false` + it('should be discarded when the method names it in `globals`', function () { const sample = { _globalOptions: { disableKeepAliveAgent: true } }; assert.strictEqual( globalize.disableKeepAliveAgent.call(sample, { globals: { disableKeepAliveAgent: false } }), + false + ); + }); + + it('should still apply where a method names other keys in `globals`', function () { + const sample = { _globalOptions: { disableKeepAliveAgent: true } }; + + assert.strictEqual( + globalize.disableKeepAliveAgent.call(sample, { globals: { before: false } }), true ); }); From 98dd3e57603288b5d0b73d9a3a69b82fe6c1b4c6 Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Thu, 3 Sep 2026 13:56:11 +0100 Subject: [PATCH 12/13] comments updated --- lib/addMethod/keepAliveAgent.js | 7 ++++++- tests/keepAliveAgent_test.js | 3 ++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/addMethod/keepAliveAgent.js b/lib/addMethod/keepAliveAgent.js index 57293c6..6a0064f 100644 --- a/lib/addMethod/keepAliveAgent.js +++ b/lib/addMethod/keepAliveAgent.js @@ -42,7 +42,12 @@ class KeepAliveAgent extends http.Agent { } createConnection (options, callback) { - //needle sets this per request: the proxy's protocol when proxying, else the target's + /* + needle sets this per request: the proxy's protocol when proxying, else + the target's. It dispatches anything but `https:` through http, so the + fallback mirrors that - a TLS socket there would break the request + rather than secure it, since needle would still be speaking http. + */ const factory = ( factories[options.protocol] || factories['http:'] ); const socket = factory.createConnection(options, callback); diff --git a/tests/keepAliveAgent_test.js b/tests/keepAliveAgent_test.js index 4f3810e..fa4ad9e 100644 --- a/tests/keepAliveAgent_test.js +++ b/tests/keepAliveAgent_test.js @@ -89,7 +89,8 @@ describe('#keepAliveAgent', function () { assert.strictEqual(connectionFor('http:'), 'http:'); }); - it('should fall back to a plain connection when the protocol is unknown', function () { + //needle dispatches anything but `https:` through http, so this must match + it('should fall back to a plain connection for any other protocol', function () { assert.strictEqual(connectionFor(undefined), 'http:'); }); From 2aadd58bee84b36e436473221bc4674023786638 Mon Sep 17 00:00:00 2001 From: Johnbastian Date: Thu, 3 Sep 2026 16:36:39 +0100 Subject: [PATCH 13/13] feedback update --- lib/addMethod/keepAliveAgent.js | 13 +++++++++++-- tests/keepAliveAgent_test.js | 10 ++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/addMethod/keepAliveAgent.js b/lib/addMethod/keepAliveAgent.js index 6a0064f..4cc54ce 100644 --- a/lib/addMethod/keepAliveAgent.js +++ b/lib/addMethod/keepAliveAgent.js @@ -22,10 +22,19 @@ const logger = require('../logger'); */ const KEEPALIVE_DELAY = 300000; -//Real agents, used only for their `createConnection`, so TLS setup stays Node's job +/* +* Real agents, used only for their `createConnection`, so TLS setup stays Node's +* job. These are shared, so `maxCachedSessions: 0` keeps them stateless: this +* agent extends http.Agent, so the TLS session key is computed by +* `http.Agent.getName` and omits `rejectUnauthorized`, `ca` and the rest. A +* session opened by a lax request would then be offered to a strict one for the +* same host. Node still verifies the certificate on resumption, so that is not +* a bypass today - but nothing here needs session reuse, and this way it cannot +* become one. +*/ const factories = { 'http:': new http.Agent({ keepAlive: false }), - 'https:': new https.Agent({ keepAlive: false }) + 'https:': new https.Agent({ keepAlive: false, maxCachedSessions: 0 }) }; class KeepAliveAgent extends http.Agent { diff --git a/tests/keepAliveAgent_test.js b/tests/keepAliveAgent_test.js index fa4ad9e..4cc2e26 100644 --- a/tests/keepAliveAgent_test.js +++ b/tests/keepAliveAgent_test.js @@ -113,6 +113,16 @@ describe('#keepAliveAgent', function () { ); }); + /* + The shared https factory keys its TLS session cache with + `http.Agent.getName`, which omits TLS options, so a lax session would be + offered to a strict request for the same host. Nothing here needs + session reuse. + */ + it('should not cache TLS sessions across requests', function () { + assert.strictEqual(resolveKeepAliveAgent.factories['https:'].maxCachedSessions, 0); + }); + it('should return a new agent per request', function () { assert.notStrictEqual(resolveKeepAliveAgent({}), resolveKeepAliveAgent({})); });