Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various fixes #5

Merged
merged 10 commits into from
Feb 13, 2019
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Change Log

- [#5] Allow for call-level age overrides as specified in the README.
- [#4] Bump various dev-dependencies
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this PR make #4 useless?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, unless we want them to be separate and more targeted in scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the merge separately.


### 1.1.0

- [#2] Conditionally ignore cache
Expand All @@ -14,3 +17,5 @@

[#1]: https://github.com/godaddy/out-of-band-cache/pull/1
[#2]: https://github.com/godaddy/out-of-band-cache/pull/2
[#4]: https://github.com/godaddy/out-of-band-cache/pull/4
[#5]: https://github.com/godaddy/out-of-band-cache/pull/5
85 changes: 55 additions & 30 deletions lib/multi-level.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,24 @@
const debug = require('diagnostics')('out-of-band-cache:multi-level');


/**
* Gets a property from either the options object if defined there, otherwise the default value.
*
* @param {Object} opts An options object
* @param {String} key The name of the property to get
* @param {Any} defaultValue The default value
*
* @private
* @returns {Any} The value of the property
*/
function getValue(opts, key, defaultValue) {
if (key in opts) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a must, but could be condensed:

return (key in opts) ? opts[key] : defaultValue;

return opts[key];
}

return defaultValue;
}

/**
* @typedef Cache
* @prop {AsyncFunction} init Initialization function
Expand All @@ -17,11 +38,11 @@
/**
* A multi-level cache.
*
* @param {Object} opts - Options for the Client instance
* @param {String} [opts.maxAge=600,000] - The duration, in milliseconds, before a cached item expires
* @param {String} [opts.maxStaleness=0] - The duration, in milliseconds, in which expired cache items are still served
* @param {ShouldCache} [opts.shouldCache=()=>true] - Function to determine whether to not we should cache the result
* @param {Cache[]} opts.caches - An Array of cache objects. See `./fs` and `./memory` for sample caches.
* @param {Object} opts Options for the Client instance
* @param {Number} [opts.maxAge=600,000] The duration, in milliseconds, before a cached item expires

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comma should probably be removed here, as it isn't in the actual default. (We might want to include a version of the default with the comma in the description for readability).

* @param {Number} [opts.maxStaleness=0] The duration, in milliseconds, in which expired cache items are still served
* @param {ShouldCache} [opts.shouldCache=()=>true] Function to determine whether to not we should cache the result
* @param {Cache[]} opts.caches An Array of cache objects. See `./fs` and `./memory` for sample caches.
*/
class MultiLevelCache {
constructor(opts) {
Expand All @@ -36,28 +57,26 @@ class MultiLevelCache {
this.shouldCache = opts.shouldCache || (() => true);

this._pendingRefreshes = {};
this._initTask = Promise.all(opts.caches.map(cache => {
return cache.init().catch(err => {
throw err;
});
}));
this._initTask = Promise.all(opts.caches.map(cache => cache.init()));
}

/**
* Attempts a cache get
*
* @param {String} key - The cache key
* @param {Object} opts - Options for this particular read
* @param {Boolean} [opts.skipCache=false] - Whether the cache should be bypassed (default false)
* @param {String} [opts.maxAge] - The duration in milliseconds before a cached item expires
* @param {ShouldCache} [opts.shouldCache] - A function to determine whether or not we should cache the item
* @param {UpdateFn} updateFn - async function that defines how to get a new value
* @param {String} key The cache key
* @param {Object} [opts={}] Options for this particular read
* @param {Boolean} [opts.skipCache=false] Whether the cache should be bypassed (default false)
* @param {String} [opts.maxAge] The duration in milliseconds before a cached item expires
* @param {Number} [opts.maxStaleness=0] The duration, in milliseconds, in which expired cache items are still served
* @param {ShouldCache} [opts.shouldCache] A function to determine whether or not we should cache the item
* @param {UpdateFn} updateFn async function that defines how to get a new value
*
* @async
* @returns {Promise<GetResult>} a Promise which resolves to an object containing
* a `value` property and a `fromCache` boolean indicator.
*/
async get(key, opts, updateFn) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like optional-options could be used to avoid having to pass in an empty object. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not a callback though, and while it has the same shape as what optional-options wants, its semantically different. Really the right thing to do is making a breaking change and reverse the order of the params. We should consider doing that at some point. Not sure its warranted just yet though. something to bear in mind (I'll open an issue)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opts = opts || {};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not put the default on the argument?

async get(key, opts = {}, updateFn)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it's an awkward API and requires you to do:

.get(myKey, undefined, someFn);

since passing a null doesn't invoke the param default.

if (opts.skipCache) {
const value = await updateFn(key, null);
return {
Expand All @@ -73,16 +92,20 @@ class MultiLevelCache {
return getChain.catch(() => cache.get(key));
}, Promise.reject());
} catch (e) { // cache miss
return this._refresh(key, null, updateFn, opts.shouldCache);
return this._refresh(key, null, updateFn, opts);
}

// cache hit
const now = Date.now();
if (item.expiry < now) {
const refreshTask = this._refresh(key, item, updateFn, opts.shouldCache);
if (item.expiry + this._maxStaleness < now) {
return refreshTask;
if (item.expiry + getValue(opts, 'maxStaleness', this._maxStaleness) < now) {
return this._refresh(key, item, updateFn, opts);
}

// Update the cache, but ignore failures
this._refresh(key, item, updateFn, opts).catch(err => {
debug('background refresh failed for %s with %s', key && JSON.stringify(key), err && err.message);
});
}

return {
Expand All @@ -103,16 +126,18 @@ class MultiLevelCache {

/**
* Refresh the cache for a given key value pair
* @param {String} key cache key
* @param {JSONSerializable} staleItem cache value
* @param {UpdateFn} updateFn async function that defines how to get a new value
* @param {ShouldCache} [shouldCache] function that determines whether or not we should cache the item
* @param {String} key cache key
* @param {JSONSerializable} staleItem cache value
* @param {UpdateFn} updateFn async function that defines how to get a new value
* @param {Object} opts An options object
* @param {ShouldCache} [opts.shouldCache] Function that determines whether or not we should cache the item
* @param {Number} [opts.maxAge] The duration, in milliseconds, before a cached item expires
*
* @private
* @async
* @returns {Promise<any>} a promise that resolves once we have refreshed the correct key
*/
async _refresh(key, staleItem, updateFn, shouldCache) {
async _refresh(key, staleItem, updateFn, opts) {
msluther marked this conversation as resolved.
Show resolved Hide resolved
if (!(key in this._pendingRefreshes)) {
try {
const task = updateFn(key, staleItem && staleItem.value);
Expand All @@ -121,16 +146,17 @@ class MultiLevelCache {
const value = await task;
const cacheItem = {
value,
expiry: Date.now() + this._maxAge
expiry: Date.now() + getValue(opts, 'maxAge', this._maxAge)
};

if (shouldCache && typeof shouldCache !== 'function') {
const { shouldCache = this.shouldCache } = opts;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be stylistically consistent, just use getValue again

const shouldCache = getValue(opts, 'shouldCache', this.shouldCache);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure


if (typeof shouldCache !== 'function') {
throw new TypeError('shouldCache has to be a function');
}

const willCache = shouldCache || this.shouldCache;
// Given that we are not ignoring this value, perform an out-of-band cache update
if (willCache(cacheItem.value)) {
if (shouldCache(cacheItem.value)) {
// NB: an in-band update would `await` this Promise.all block
Promise.all(this._caches.map(cache => cache.set(key, cacheItem))).catch(err => {
throw new Error(`Error caching ${key}`, err);
Expand All @@ -143,7 +169,6 @@ class MultiLevelCache {
}

return { value, fromCache: false };

} catch (err) {
delete this._pendingRefreshes[key];
throw err;
Expand Down
Loading