CNC-1364: revert and fix keep alive agent

.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',

lib/addMethod/addMethodREST.js

@@ -14,6 +14,7 @@ var validateNotExpects	= require('./validateNotExpects');
 var formatResponse = require('./formatResponse');
 
 var addMethodFunction	= require('./addMethodFunction');
+var resolveKeepAliveAgent = require('./keepAliveAgent');
 
 module.exports = function (methodName, config, afterHeadersFunction) {
 	var threadneedle = this;
@@ -29,15 +30,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
@@ -222,6 +219,13 @@ 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 = resolveKeepAliveAgent(
+						request.options,
+						globalize.disableKeepAliveAgent.call(threadneedle, config)
+					);
+
 					// Run a different method for get to not include data
 					switch (method) {
 						case 'get':

lib/addMethod/globalize/disableKeepAliveAgent.js

@@ -0,0 +1,34 @@
+/*
+* 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.
+*
+* 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');
+
+module.exports = function (config) {
+
+	if (_.isBoolean(config.disableKeepAliveAgent)) {
+		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;
+	}
+
+	return false;
+
+};

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')
 };

lib/addMethod/keepAliveAgent.js

@@ -0,0 +1,94 @@
+/*
+* 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
+*
+* 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.
+*
+* Switch off with `disableKeepAliveAgent` - see globalize/disableKeepAliveAgent.js
+*/
+const http  = require('http');
+const https = require('https');
+
+const logger = require('../logger');
+
+/*
+* 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;
+
+/*
+* 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, maxCachedSessions: 0 })
+};
+
+class KeepAliveAgent extends http.Agent {
+
+	constructor (options) {
+		super(options);
+
+		/*
+		  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) {
+		/*
+		  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);
+		socket.setKeepAlive(true, KEEPALIVE_DELAY); //set SO_KEEPALIVE
+
+		return socket;
+	}
+
+}
+
+//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) {
+		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;
+	}
+
+	/*
+	  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 });
+
+};
+
+module.exports.KEEPALIVE_DELAY = KEEPALIVE_DELAY;
+module.exports.KeepAliveAgent = KeepAliveAgent;
+module.exports.factories = factories;

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": {

tests/addMethodREST_test.js

@@ -109,8 +109,7 @@ describe('#addMethodREST', function () {
 		});
 
 		after(function (done) {
-			server.close();
-			done();
+			server.close(done);
 		});
 
 		var threadneedle;

tests/globalize_test.js

@@ -2067,4 +2067,83 @@ 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
+			);
+		});
+
+		//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 } };
+
+			assert.strictEqual(
+				globalize.disableKeepAliveAgent.call(sample, { globals: false }),
+				true
+			);
+		});
+
+		//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
+			);
+		});
+
+	});
+
+
 });