feat: True timeouts for cache calls

Author: Jan Krems

README.md

@@ -138,6 +138,11 @@ The two important ones are `freshFor` and `expire`:
 
 * `expire` is the time in seconds after which a value should be deleted from the cache (or whatever expiring natively means for the backend). Usually you'd want this to be `0` (never expire).
 * `freshFor` is the time in seconds after which a value should be replaced. Replacing the value is done in the background and while the new value is generated (e.g. data is fetched from some service) the stale value is returned. Think of `freshFor` as a smarter `expire`.
+* `timeout` is the maximum time in milliseconds to wait for cache operations to complete.
+  Configuring a timeout ensures that all `get` and `set` operations fail fast.
+  Otherwise there will be situations where one of the cache hosts goes down and reads hang for minutes while the memcached client retries to establish a connection.
+  It's **highly** recommended to set a timeout.
+  If `timeout` is left `undefined`, no timeout will be set and the operations will only fail once the underlying client, e.g. [`memcached`](https://github.com/3rd-Eden/memcached), gave up.
 
 ### Cache.set(key, value, opts, cb) -> Promise[Value]
 

lib/cache.js

@@ -87,7 +87,7 @@ Cache.prototype._set = function _set(key, val, options) {
     }, options);
   }
 
-  return util.toPromise(val).then(writeToBackend);
+  return this._applyTimeout(util.toPromise(val).then(writeToBackend));
 };
 
 Cache.prototype.set = function set(rawKey, val, _opts, _cb) {
@@ -98,8 +98,16 @@ Cache.prototype.set = function set(rawKey, val, _opts, _cb) {
   return this._set(key, val, optsWithDefaults).nodeify(args.cb);
 };
 
+Cache.prototype._applyTimeout = function _applyTimeout(value) {
+  var timeoutMs = this.defaults.timeout;
+  if (timeoutMs > 0) {
+    return value.timeout(timeoutMs);
+  }
+  return value;
+};
+
 Cache.prototype._getWrapped = function _getWrapped(key) {
-  return this.backend.get(key);
+  return this._applyTimeout(this.backend.get(key));
 };
 // For backwards compatibility, eventually we should deprecate this.
 // It *should* be a private API.

test/timeout.test.js

@@ -0,0 +1,80 @@
+import assert from 'assertive';
+import Bluebird from 'bluebird';
+import { identity } from 'lodash';
+
+import Cache from '../lib/cache';
+
+describe('Cache timeouts', () => {
+  const cache = new Cache({
+    backend: {
+      get() {
+        return Bluebird.resolve({ d: 'get result' }).delay(150);
+      },
+      set() {
+        return Bluebird.resolve('set result').delay(150);
+      },
+    },
+    name: 'awesome-name',
+    debug: true,
+  });
+
+  describe('with a timeout <150ms', () => {
+    before(() => cache.defaults.timeout = 50);
+
+    it('get fails fast', async () => {
+      const err = await Bluebird.race([
+        cache.get('my-key').then(null, identity),
+        Bluebird.delay(100, 'too slow'), // this should not be used
+      ]);
+      assert.expect(err instanceof Error);
+      assert.equal('TimeoutError', err.name);
+    });
+
+    it('set fails fast', async () => {
+      const err = await Bluebird.race([
+        cache.set('my-key', 'my-value').then(null, identity),
+        Bluebird.delay(100, 'too slow'), // this should not be used
+      ]);
+      assert.expect(err instanceof Error);
+      assert.equal('TimeoutError', err.name);
+    });
+
+    it('getOrElse fails fast', async () => {
+      const value = await Bluebird.race([
+        cache.getOrElse('my-key', 'my-value').then(null, identity),
+        // We need to add a bit of time here because we'll run into the
+        // timeout twice - once when trying to read and once while writing.
+        Bluebird.delay(150, 'too slow'), // this should not be used
+      ]);
+      assert.equal('my-value', value);
+    });
+  });
+
+  describe('with a timeout >150ms', () => {
+    before(() => cache.defaults.timeout = 250);
+
+    it('receives the value', async () => {
+      const value = await Bluebird.race([
+        cache.get('my-key').then(null, identity),
+        Bluebird.delay(200, 'too slow'), // this should not be used
+      ]);
+      assert.equal('get result', value);
+    });
+
+    it('sets the value', async () => {
+      const value = await Bluebird.race([
+        cache.set('my-key', 'my-value').then(null, identity),
+        Bluebird.delay(200, 'too slow'), // this should not be used
+      ]);
+      assert.equal('set result', value);
+    });
+
+    it('getOrElse can retrieve a value', async () => {
+      const value = await Bluebird.race([
+        cache.getOrElse('my-key', 'my-value').then(null, identity),
+        Bluebird.delay(200, 'too slow'), // this should not be used
+      ]);
+      assert.equal('get result', value);
+    });
+  });
+});