Simplify code by always adding to the cache

Signed-off-by: Gavin Halliday <[email protected]>

system/jlib/jsecrets.cpp

@@ -453,44 +453,37 @@ class SecretCacheEntry : public CInterface
 class SecretCache
 {
 public:
-    SecretCacheEntry * addSecret(const std::string & secretKey, IPropertyTree * value, cache_timestamp now)
+    const IPropertyTree * getContents(SecretCacheEntry * match)
     {
+        //Return contents within the critical section so no other thread can modify it
         CriticalBlock block(cs);
-        //Need to check if another thread has added the secret - because we need to guarantee that
-        //the unique pointers remain alive - and unconditionally replacing would break that.
-        auto match = secrets.find(secretKey);
+        return LINK(match->contents);
+    }
+
+    //Check to see if a secret exists, and if not add a null entry that has expired.
+    SecretCacheEntry * resolveSecret(const std::string & secretKey, cache_timestamp now)
+    {
         SecretCacheEntry * result;
+        CriticalBlock block(cs);
+        auto match = secrets.find(secretKey);
         if (match != secrets.cend())
         {
             result = match->second.get();
-            result->updateContents(value, now);
+            result->accessedTimestamp = now;
         }
         else
         {
-            result = new SecretCacheEntry(value, now);
-            secrets.emplace(secretKey, result );
+            //Insert an entry with a null value that is marked as out of date
+            result = new SecretCacheEntry(nullptr, now - 2 * secretTimeoutMs);
+            secrets.emplace(secretKey, result);
         }
         return result;
     }
 
-    const IPropertyTree * getContents(SecretCacheEntry * match)
-    {
-        //Return contents within the critical section so no other thread can modify it
-        CriticalBlock block(cs);
-        return LINK(match->contents);
-    }
-
-    SecretCacheEntry * querySecret(const std::string & secretKey, cache_timestamp now)
+    void updateSecret(SecretCacheEntry * match, IPropertyTree * value, cache_timestamp now)
     {
         CriticalBlock block(cs);
-        auto match = secrets.find(secretKey);
-        if (match != secrets.cend())
-        {
-            match->second->accessedTimestamp = now;
-            return match->second.get();
-        }
-        else
-            return nullptr;
+        match->updateContents(value, now);
     }
 
 private:
@@ -1079,8 +1072,8 @@ static SecretCacheEntry * getSecretEntry(const char *category, const char * name
     if (optVersion)
         key.append("#").append(optVersion);
 
-    SecretCacheEntry * match = globalSecretCache.querySecret(key, now);
-    if (match && !match->needsRefresh(now))
+    SecretCacheEntry * match = globalSecretCache.resolveSecret(key, now);
+    if (!match->needsRefresh(now))
         return match;
 
     Owned<IPropertyTree> resolved;
@@ -1099,8 +1092,8 @@ static SecretCacheEntry * getSecretEntry(const char *category, const char * name
     }
 
     //If the secret could no longer be resolved (e.g. a vault has gone down) then keep the old one
-    if (resolved || !match)
-        match = globalSecretCache.addSecret(key, resolved, now);
+    if (resolved)
+        globalSecretCache.updateSecret(match, resolved, now);
     else
         match->noteFailedUpdate(now);
 
@@ -1229,6 +1222,8 @@ void CSecret::checkUptoDate() const
         try
         {
             SecretCacheEntry * newSecret = getSecretEntry(category, name, vaultId, version);
+            //Check the secret is always returned consistently.  It would be possible to call a slightly
+            //more optimal function to refresh a secret, but this is simplest.
             assertex(secret == newSecret);
             updateHash();
         }