Merge pull request #20301 from ghalliday/issue34805

HPCC-34805 Fix race condition on first access to a secret

Reviewed-By: Jack Del Vecchio <Jack.DelVecchio@lexisnexisrisk.com>
Merged-by: Gavin Halliday <gavin.halliday@lexisnexisrisk.com>

Author: ghalliday

system/jlib/jsecrets.cpp

@@ -465,6 +465,8 @@ class SecretCacheEntry : public CInterface
     // Is it time to check if there is a new value for this secret?
     bool needsRefresh(cache_timestamp now) const
     {
+        if (!initialized)
+            return true;
         cache_timestamp elapsed = (now - checkedTimestamp);
         return (elapsed > secretTimeoutNs);
     }
@@ -479,6 +481,7 @@ class SecretCacheEntry : public CInterface
         //Update the checked timestamp - so that we do not continually check for updates to secrets which
         //are stale because the vault or other source of values in inaccessible.
         //Keep using the last good value
+        initialized = true;
         checkedTimestamp = now;
         if (accessed)
             accessedTimestamp = now;
@@ -490,6 +493,7 @@ class SecretCacheEntry : public CInterface
 private:
     void updateContents(IPropertyTree * _contents, cache_timestamp now, bool accessed)
     {
+        initialized = true;
         contents.set(_contents);
         updateHash();
         contentTimestamp = now;
@@ -512,6 +516,7 @@ class SecretCacheEntry : public CInterface
     cache_timestamp accessedTimestamp = 0; // When was this secret last accessed?
     cache_timestamp checkedTimestamp = 0;  // When was this last checked for updates?
     unsigned contentHash = 0;
+    bool initialized = false;
 };
 
 

testing/unittests/jlibtests.cpp

@@ -4519,6 +4519,7 @@ class JLibSecretsTest : public CppUnit::TestFixture
 public:
     CPPUNIT_TEST_SUITE(JLibSecretsTest);
         CPPUNIT_TEST(setup);
+        CPPUNIT_TEST(testConcurrentRead);
         CPPUNIT_TEST(testUpdate1);
         CPPUNIT_TEST(testUpdate2);
         CPPUNIT_TEST(testBackgroundUpdate);
@@ -4789,6 +4790,66 @@ class JLibSecretsTest : public CppUnit::TestFixture
         stopSecretUpdateThread();
     }
 
+    class SecretReader : public Thread
+    {
+    public:
+        SecretReader(Semaphore & _startSem) : startSem(_startSem) {}
+
+        virtual int run()
+        {
+            startSem.wait();
+            try
+            {
+                getSecretValue(secret, "testing", "concurrent", "value", true);
+            }
+            catch (IException * e)
+            {
+                DBGLOG("Failed to read secret");
+                error.setown(e);
+            }
+            return 0;
+        }
+
+    public:
+        Semaphore & startSem;
+        StringBuffer secret;
+        Owned<IException> error;
+    };
+
+    void testConcurrentRead()
+    {
+        initPath(); // secretRoot needs to be called for each test
+
+        writeTestingSecret("concurrent", "value", "hello");
+
+        unsigned numReaders = 5;
+        Semaphore startSem;
+        CIArrayOf<SecretReader> readers;
+
+        for (unsigned i = 0; i < numReaders; i++)
+        {
+            SecretReader * reader = new SecretReader(startSem);
+            readers.append(*reader);
+            reader->start(true);
+        }
+
+        startSem.signal(numReaders);
+
+        for (unsigned i = 0; i < numReaders; i++)
+            readers.item(i).join();
+
+        for (unsigned i = 0; i < numReaders; i++)
+        {
+            IException * e = readers.item(i).error;
+            if (e)
+            {
+                StringBuffer msg;
+                e->errorMessage(msg);
+                CPPUNIT_FAIL(msg.str());
+            }
+        }
+    }
+
     void testKeyEncoding()
     {
         for (auto category : { "abc", "def" })