Browse Source

HPCC-25945 Simplify config update hook installation

Signed-off-by: Jake Smith <jake.smith@lexisnexisrisk.com>
Jake Smith 4 years ago
parent
commit
71c9a16259

+ 6 - 20
dali/base/dafdesc.cpp

@@ -3339,17 +3339,8 @@ static void generateHosts(IPropertyTree * storage, GroupInfoArray & groups)
     }
 }
 
-
+static CConfigUpdateHook configUpdateHook;
 static std::atomic<unsigned> normalizeHostGroupUpdateCBId{(unsigned)-1};
-MODULE_INIT(INIT_PRIORITY_STANDARD)
-{
-    return true;
-}
-MODULE_EXIT()
-{
-    if ((unsigned)-1 != normalizeHostGroupUpdateCBId)
-        removeConfigUpdateHook(normalizeHostGroupUpdateCBId);
-}
 static CriticalSection storageCS;
 static void doInitializeStorageGroups(bool createPlanesFromGroups)
 {
@@ -3463,17 +3454,12 @@ static void doInitializeStorageGroups(bool createPlanesFromGroups)
 
 void initializeStorageGroups(bool createPlanesFromGroups)
 {
-    doInitializeStorageGroups(createPlanesFromGroups);
-    unsigned uninitialized = (unsigned)-1;
-    if (normalizeHostGroupUpdateCBId.compare_exchange_strong(uninitialized, 0))
+    auto updateFunc = [createPlanesFromGroups](const IPropertyTree *oldComponentConfiguration, const IPropertyTree *oldGlobalConfiguration)
     {
-        auto updateFunc = [createPlanesFromGroups](const IPropertyTree *oldComponentConfiguration, const IPropertyTree *oldGlobalConfiguration)
-        {
-            PROGLOG("initializeStorageGroups update");
-            doInitializeStorageGroups(createPlanesFromGroups);
-        };
-        normalizeHostGroupUpdateCBId = installConfigUpdateHook(updateFunc);
-    }
+        PROGLOG("initializeStorageGroups update");
+        doInitializeStorageGroups(createPlanesFromGroups);
+    };
+    configUpdateHook.installOnce(updateFunc, true);
 }
 
 bool getDefaultStoragePlane(StringBuffer &ret)

+ 3 - 4
ecl/eclccserver/eclccserver.cpp

@@ -920,7 +920,7 @@ class EclccServer : public CInterface, implements IThreadFactory, implements IAb
     Owned<IJobQueue> queue;
     CriticalSection queueUpdateCS;
     StringAttr updatedQueueNames;
-    unsigned reloadConfigCBId = 0;
+    CConfigUpdateHook reloadConfigHook;
 
 
     void configUpdate()
@@ -951,13 +951,12 @@ public:
         pool.setown(createThreadPool("eclccServerPool", this, NULL, poolSize, INFINITE));
         serverstatus.queryProperties()->setProp("@cluster", getComponentConfigSP()->queryProp("@name"));
         serverstatus.commitProperties();
-        reloadConfigCBId = installConfigUpdateHook(std::bind(&EclccServer::configUpdate, this));
+        reloadConfigHook.installOnce(std::bind(&EclccServer::configUpdate, this), false);
     }
 
     ~EclccServer()
     {
-        if (reloadConfigCBId)
-            removeConfigUpdateHook(reloadConfigCBId);
+        reloadConfigHook.clear();
         pool->joinAll(false, INFINITE);
     }
 

+ 2 - 15
esp/smc/SMCLib/TpWrapper.cpp

@@ -29,16 +29,7 @@
 #include "dautils.hpp"
 #include "dameta.hpp"
 
-static unsigned reloadConfigCBId = (unsigned)-1;
-MODULE_INIT(INIT_PRIORITY_STANDARD)
-{
-    return true;
-}
-MODULE_EXIT()
-{
-    if ((unsigned)-1 != reloadConfigCBId)
-        removeConfigUpdateHook(reloadConfigCBId);
-}
+static CConfigUpdateHook configUpdateHook;
 
 const char* MSG_FAILED_GET_ENVIRONMENT_INFO = "Failed to get environment information.";
 
@@ -2441,11 +2432,7 @@ extern TPWRAPPER_API void validateTargetName(const char* target)
 
     CriticalBlock block(validTargetSect);
 #ifdef _CONTAINERIZED
-    if ((unsigned) -1 == reloadConfigCBId) // once only
-    {
-        refreshValidTargets();
-        reloadConfigCBId = installConfigUpdateHook(configUpdate);
-    }
+    configUpdateHook.installOnce(configUpdate, true);
     if (validTargets.find(target) == validTargets.end())
         throw makeStringExceptionV(ECLWATCH_INVALID_CLUSTER_NAME, "Invalid target name: %s", target);
 #else

+ 36 - 1
system/jlib/jptree.cpp

@@ -8612,7 +8612,17 @@ public:
                 /* NB: we are still holding 'configCS' at this point, blocking all other thread access.
                    However code in callbacks may call e.g. getComponentConfig() and re-enter the crit */
                 for (const auto &item: notifyConfigUpdates)
-                    item.second(oldComponentConfiguration, oldGlobalConfiguration);
+                {
+                    try
+                    {
+                        item.second(oldComponentConfiguration, oldGlobalConfiguration);
+                    }
+                    catch (IException *e)
+                    {
+                        EXCLOG(e, "CConfigUpdater callback");
+                        e->Release();
+                    }
+                }
 
                 absoluteConfigFilename.set(std::get<0>(result).c_str());
             }
@@ -8677,6 +8687,31 @@ void removeConfigUpdateHook(unsigned notifyFuncId)
 }
 #endif // __linux__
 
+void CConfigUpdateHook::clear()
+{
+    unsigned id = configCBId.exchange((unsigned)-1);
+    if ((unsigned)-1 != id)
+        removeConfigUpdateHook(id);
+}
+
+void CConfigUpdateHook::installOnce(ConfigUpdateFunc callbackFunc, bool callWhenInstalled)
+{
+    unsigned id = configCBId.load(std::memory_order_acquire);
+    if ((unsigned)-1 == id) // avoid CS in common case
+    {
+        CriticalBlock b(crit);
+        // check again now in CS
+        id = configCBId.load(std::memory_order_acquire);
+        if ((unsigned)-1 == id)
+        {
+            if (callWhenInstalled)
+                callbackFunc(getComponentConfigSP(), getGlobalConfigSP());
+            id = installConfigUpdateHook(callbackFunc);
+            configCBId.store(id, std::memory_order_release);
+        }
+    }
+}
+
 
 static std::tuple<std::string, IPropertyTree *, IPropertyTree *> doLoadConfiguration(IPropertyTree *componentDefault, const char * * argv, const char * componentTag, const char * envPrefix, const char * legacyFilename, IPropertyTree * (mapper)(IPropertyTree *), const char *altNameAttribute)
 {

+ 10 - 0
system/jlib/jptree.hpp

@@ -324,6 +324,16 @@ typedef std::function<void (const IPropertyTree *oldComponentConfiguration, cons
 jlib_decl unsigned installConfigUpdateHook(ConfigUpdateFunc notifyFunc);
 jlib_decl void removeConfigUpdateHook(unsigned notifyFuncId);
 
+class jlib_decl CConfigUpdateHook
+{
+    std::atomic<unsigned> configCBId{(unsigned)-1};
+    CriticalSection crit;
+public:
+    ~CConfigUpdateHook() { clear(); }
+    void clear();
+    void installOnce(ConfigUpdateFunc callbackFunc, bool callWhenInstalled);
+};
+
 /*
  YAML to PTree support
    By default YAML scalars become PTree attributes unless the YAML has an !element or !el YAML tag specifying that the scalar