Merge branch 'master' into add_crunch

Author: mzikherman

src/config.js

@@ -7,6 +7,7 @@ const {
   BIDDER_POSITION_MAX_BID_AMOUNT_CENTS_LIMIT,
   CACHE_LIFETIME_IN_SECONDS,
   CACHE_QUERY_LOGGING_THRESHOLD_MS,
+  CACHE_RETRIEVAL_TIMEOUT_MS,
   CONVECTION_API_BASE,
   CONVECTION_APP_ID,
   CONVECTION_GEMINI_TEMPLATE,
@@ -86,6 +87,7 @@ export default {
     Number(BIDDER_POSITION_MAX_BID_AMOUNT_CENTS_LIMIT) ||
     Number.MAX_SAFE_INTEGER,
   CACHE_LIFETIME_IN_SECONDS: Number(CACHE_LIFETIME_IN_SECONDS) || 2592000, // 30 days
+  CACHE_RETRIEVAL_TIMEOUT_MS: Number(CACHE_RETRIEVAL_TIMEOUT_MS) || 2000,
   CONVECTION_API_BASE,
   CONVECTION_APP_ID,
   CONVECTION_GEMINI_TEMPLATE,
@@ -129,7 +131,8 @@ export default {
   REDIS_URL,
   REQUEST_THROTTLE_MS: Number(REQUEST_THROTTLE_MS) || 5000,
   REQUEST_TIMEOUT_MS: Number(REQUEST_TIMEOUT_MS) || 5000,
-  CACHE_QUERY_LOGGING_THRESHOLD_MS: Number(CACHE_QUERY_LOGGING_THRESHOLD_MS) || 1000,
+  CACHE_QUERY_LOGGING_THRESHOLD_MS:
+    Number(CACHE_QUERY_LOGGING_THRESHOLD_MS) || 1000,
   RESIZING_SERVICE,
   SENTRY_PRIVATE_DSN,
 }

src/lib/__tests__/cache.test.js

@@ -18,7 +18,7 @@ describe("Cache", () => {
       it("deletes the data", () => {
         cache.delete("get_foo")
         return cache.get("get_foo").catch(e => {
-          expect(e.message).toEqual("cache#get did not return `data`")
+          expect(e.message).toEqual("[Cache#get] Cache miss")
         })
       })
     })

src/lib/cache.js

@@ -9,6 +9,7 @@ const {
   REDIS_URL,
   CACHE_LIFETIME_IN_SECONDS,
   CACHE_QUERY_LOGGING_THRESHOLD_MS,
+  CACHE_RETRIEVAL_TIMEOUT_MS,
 } = config
 
 const isTest = NODE_ENV === "test"
@@ -40,13 +41,13 @@ function createRedisClient() {
         // End reconnecting on a specific error and flush all commands with a
         // individual error.
         if (options.error.code === "ECONNREFUSED") {
-          return new Error("The server refused the connection")
+          return new Error("[Cache] The server refused the connection")
         }
       }
       // End reconnecting after a specific timeout and flush all commands with a
       // individual error.
       if (options.total_retry_time > 1000 * 60 * 60) {
-        return new Error("Retry time exhausted")
+        return new Error("[Cache] Retry time exhausted")
       }
       // End reconnecting with built in error.
       if (options.attempt > 10) {
@@ -61,7 +62,7 @@ function createRedisClient() {
   }
   client.on("error", error)
   VerboseEvents.forEach(event => {
-    client.on(event, () => verbose(`Redis: ${event}`))
+    client.on(event, () => verbose(`[Cache] ${event}`))
   })
   return client
 }
@@ -71,16 +72,37 @@ export const client = isTest ? createMockClient() : createRedisClient()
 export default {
   get: key => {
     return new Promise((resolve, reject) => {
-      if (isNull(client)) return reject(new Error("Cache client is `null`"))
+      if (isNull(client)) return reject(new Error("[Cache] `client` is `null`"))
+
+      let timeoutId = setTimeout(() => {
+        timeoutId = null
+        const err = new Error(`[Cache#get] Timeout for key ${key}`)
+        error(err)
+        reject(err)
+      }, CACHE_RETRIEVAL_TIMEOUT_MS)
+
       const start = Date.now()
       client.get(key, (err, data) => {
         const time = Date.now() - start
         if (time > CACHE_QUERY_LOGGING_THRESHOLD_MS) {
-          error(`Slow Cache#Get: ${time}ms`)
+          error(`[Cache#get] Slow read of ${time}ms for key ${key}`)
+        }
+
+        if (timeoutId) {
+          clearTimeout(timeoutId)
+        } else {
+          // timed out and already rejected promise, no need to continue
+          return
+        }
+
+        if (err) {
+          error(err)
+          reject(err)
+        } else if (data) {
+          resolve(JSON.parse(data))
+        } else {
+          reject(new Error("[Cache#get] Cache miss"))
         }
-        if (err) return reject(err)
-        if (data) return resolve(JSON.parse(data))
-        reject(new Error("cache#get did not return `data`"))
       })
     })
   },