handle failures from fetchMethod

Remove the failed fetch key from the cache, rather than raising an
unhandled rejection error when the promise was not returned.

At present, there is no way to suppress this behavior, but it is
still preferable to raising an unhandled exception in these cases.

Fix: isaacs#233

CHANGELOG.md

@@ -4,6 +4,9 @@
 
 * Better AbortController polyfill, supporting
   `signal.addEventListener('abort')` and `signal.onabort`.
+* (7.9.1) Drop item from cache instead of crashing with an
+  `unhandledRejection` when the `fetchMethod` throws an error or
+  returns a rejected Promise.
 
 ## 7.8.0
 

index.js

@@ -618,14 +618,25 @@ class LRUCache {
       signal: ac.signal,
       options,
     }
-    const p = Promise.resolve(this.fetchMethod(k, v, fetchOpts)).then(v => {
+    const cb = v => {
       if (!ac.signal.aborted) {
         this.set(k, v, fetchOpts.options)
       }
       return v
-    })
+    }
+    const eb = er => {
+      if (this.valList[index] === p) {
+        this.delete(k)
+      }
+      if (p.__returned === p) {
+        throw er
+      }
+    }
+    const pcall = res => res(this.fetchMethod(k, v, fetchOpts))
+    const p = new Promise(pcall).then(cb, eb)
     p.__abortController = ac
     p.__staleWhileFetching = v
+    p.__returned = null
     if (index === undefined) {
       this.set(k, p, fetchOpts.options)
       index = this.keyMap.get(k)
@@ -637,7 +648,9 @@ class LRUCache {
 
   isBackgroundFetch (p) {
     return p && typeof p === 'object' && typeof p.then === 'function' &&
-      Object.prototype.hasOwnProperty.call(p, '__staleWhileFetching')
+      Object.prototype.hasOwnProperty.call(p, '__staleWhileFetching') &&
+      Object.prototype.hasOwnProperty.call(p, '__returned') &&
+      (p.__returned === p || p.__returned === null)
   }
 
   // this takes the union of get() and set() opts, because it does both
@@ -666,13 +679,14 @@ class LRUCache {
 
     let index = this.keyMap.get(k)
     if (index === undefined) {
-      return this.backgroundFetch(k, index, options)
+      const p = this.backgroundFetch(k, index, options)
+      return (p.__returned = p)
     } else {
       // in cache, maybe already fetching
       const v = this.valList[index]
       if (this.isBackgroundFetch(v)) {
         return allowStale && v.__staleWhileFetching !== undefined
-          ? v.__staleWhileFetching : v
+          ? v.__staleWhileFetching : (v.__returned = v)
       }
 
       if (!this.isStale(index)) {
@@ -687,7 +701,7 @@ class LRUCache {
       // refresh the cache.
       const p = this.backgroundFetch(k, index, options)
       return allowStale && p.__staleWhileFetching !== undefined
-        ? p.__staleWhileFetching : p
+        ? p.__staleWhileFetching : (p.__returned = p)
     }
   }
 

test/fetch.js

@@ -231,3 +231,48 @@ t.test('fetch options, signal, with polyfill', async t => {
   const v5 = await c.fetch(2, { ttl: 1 })
   t.equal(c.getRemainingTTL(2), 25, 'overridden ttl in fetchMethod')
 })
+
+t.test('fetchMethod throws', async t => {
+  // make sure that even if there's no one to sit around and wait for it,
+  // the background fetch throwing doesn't blow anything up.
+  const cache = new LRU({
+    max: 10,
+    ttl: 10,
+    allowStale: true,
+    fetchMethod: async () => {
+      throw new Error('fetch failure')
+    },
+  })
+  // seed the cache, and make the values stale.
+  // this simulates the case where the fetch() DID work,
+  // and replaced the promise with the resolution, but
+  // then they got stale.
+  cache.set('a', 1)
+  cache.set('b', 2)
+  clock.advance(20)
+  await Promise.resolve().then(() => {})
+  const a = await Promise.all([
+    cache.fetch('a'),
+    cache.fetch('a'),
+    cache.fetch('a'),
+  ])
+  t.strictSame(a, [1, 1, 1])
+  // clock advances, promise rejects
+  clock.advance(20)
+  await Promise.resolve().then(() => {})
+  t.equal(cache.get('a'), undefined, 'removed from cache')
+  const b = await Promise.all([
+    cache.fetch('b'),
+    cache.fetch('b'),
+    cache.fetch('b'),
+  ])
+  t.strictSame(b, [2, 2, 2])
+  clock.advance(20)
+  await Promise.resolve().then(() => {})
+  t.equal(cache.get('b'), undefined, 'removed from cache')
+  const ap = cache.fetch('a')
+  cache.set('a', 99)
+  await t.rejects(ap, { message: 'fetch failure' })
+  t.equal(cache.get('a'), 99, 'did not delete new value')
+  t.rejects(cache.fetch('b'), { message: 'fetch failure' })
+})