-
Notifications
You must be signed in to change notification settings - Fork 4
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
Changes from 5 commits
eee4699
fa0d2bd
dee6964
d676561
fd2a59f
68dd06e
63c2d1c
8ae6b37
0f9705c
30a721a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
opts = opts || {}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
if (opts.skipCache) { | ||
const value = await updateFn(key, null); | ||
return { | ||
|
@@ -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 { | ||
|
@@ -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); | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to be stylistically consistent, just use const shouldCache = getValue(opts, 'shouldCache', this.shouldCache); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -143,7 +169,6 @@ class MultiLevelCache { | |
} | ||
|
||
return { value, fromCache: false }; | ||
|
||
} catch (err) { | ||
delete this._pendingRefreshes[key]; | ||
throw err; | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the merge separately.