From 0928a7a386210cd21f8c54940bc1c9c9f8b1e0f3 Mon Sep 17 00:00:00 2001 From: Dave Streeter Date: Wed, 5 Mar 2025 13:41:53 +0000 Subject: [PATCH 1/3] HPCC-33586 Fix double-checked locking pattern in Dali Change DFdir to be an atomic pointer for thread safety. Signed-off-by: Dave Streeter --- dali/base/dadfs.cpp | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/dali/base/dadfs.cpp b/dali/base/dadfs.cpp index 1526640ca49..7699c07a97b 100644 --- a/dali/base/dadfs.cpp +++ b/dali/base/dadfs.cpp @@ -8239,20 +8239,22 @@ class CNamedGroupStore: implements INamedGroupStore, public CInterface }; -static CNamedGroupStore *groupStore = NULL; +static std::atomic groupStore{nullptr}; static CriticalSection groupsect; bool CNamedGroupIterator::match() { if (conn.get()) { if (matchgroup.get()) { - if (!groupStore) + CLeavableCriticalBlock block3(groupsect); + if (!groupStore.load()) return false; const char *name = pe->query().queryProp("@name"); if (!name||!*name) return false; GroupType dummy; - Owned lgrp = groupStore->dolookup(name, conn, NULL, dummy); + Owned lgrp = groupStore.load()->dolookup(name, conn, NULL, dummy); + block3.leave(); if (lgrp) { if (exactmatch) return lgrp->equals(matchgroup); @@ -8266,14 +8268,13 @@ bool CNamedGroupIterator::match() return false; } -INamedGroupStore &queryNamedGroupStore() +INamedGroupStore &queryNamedGroupStore() { - if (!groupStore) { - CriticalBlock block(groupsect); - if (!groupStore) - groupStore = new CNamedGroupStore(); + CriticalBlock block(groupsect); + if (!groupStore.load()) { + groupStore.store(new CNamedGroupStore()); } - return *groupStore; + return *(groupStore.load()); } // -------------------------------------------------------- @@ -9244,8 +9245,8 @@ void closedownDFS() // called by dacoven } DFdir = NULL; CriticalBlock block2(groupsect); - ::Release(groupStore); - groupStore = NULL; + ::Release(groupStore.load()); + groupStore.store(nullptr); } class CDFPartFilter : implements IDFPartFilter, public CInterface From 53ebcce7d0885eaa8f8541dd82a0a90805d69199 Mon Sep 17 00:00:00 2001 From: Dave Streeter Date: Tue, 11 Mar 2025 10:45:37 +0000 Subject: [PATCH 2/3] HPCC-33586 Fix double-checked locking pattern in Dali Change DFdir to be an atomic pointer for thread safety. Signed-off-by: Dave Streeter --- dali/base/dadfs.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/dali/base/dadfs.cpp b/dali/base/dadfs.cpp index 7699c07a97b..191e5cb263f 100644 --- a/dali/base/dadfs.cpp +++ b/dali/base/dadfs.cpp @@ -9205,7 +9205,7 @@ GetFileClusterNamesType CDistributedFileDirectory::getFileClusterNames(const cha // -------------------------------------------------------- -static CDistributedFileDirectory *DFdir = NULL; +static std::atomic DFdir{nullptr}; static CriticalSection dfdirCrit; /** @@ -9215,12 +9215,10 @@ static CriticalSection dfdirCrit; */ IDistributedFileDirectory &queryDistributedFileDirectory() { - if (!DFdir) { - CriticalBlock block(dfdirCrit); - if (!DFdir) - DFdir = new CDistributedFileDirectory(); - } - return *DFdir; + CriticalBlock block(dfdirCrit); + if (!DFdir.load()) + DFdir.store(new CDistributedFileDirectory()); + return *DFdir.load(); } /** @@ -9230,7 +9228,7 @@ void closedownDFS() // called by dacoven { CriticalBlock block(dfdirCrit); try { - delete DFdir; + delete DFdir.load(); } catch (IMP_Exception *e) { if (e->errorCode()!=MPERR_link_closed) @@ -9243,7 +9241,7 @@ void closedownDFS() // called by dacoven throw; e->Release(); } - DFdir = NULL; + DFdir.store(nullptr); CriticalBlock block2(groupsect); ::Release(groupStore.load()); groupStore.store(nullptr); From 62f2415f8babde91140e2f8a8b2033ec6b369be1 Mon Sep 17 00:00:00 2001 From: Dave Streeter Date: Tue, 11 Mar 2025 16:37:46 +0000 Subject: [PATCH 3/3] HPCC-33586 Fix double-checked locking pattern in Dali As per PR comments remove unnecessary CriticalSection Signed-off-by: Dave Streeter --- dali/base/dadfs.cpp | 47 +++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/dali/base/dadfs.cpp b/dali/base/dadfs.cpp index 191e5cb263f..dbd1738589c 100644 --- a/dali/base/dadfs.cpp +++ b/dali/base/dadfs.cpp @@ -8240,13 +8240,11 @@ class CNamedGroupStore: implements INamedGroupStore, public CInterface }; static std::atomic groupStore{nullptr}; -static CriticalSection groupsect; bool CNamedGroupIterator::match() { if (conn.get()) { if (matchgroup.get()) { - CLeavableCriticalBlock block3(groupsect); if (!groupStore.load()) return false; const char *name = pe->query().queryProp("@name"); @@ -8254,7 +8252,6 @@ bool CNamedGroupIterator::match() return false; GroupType dummy; Owned lgrp = groupStore.load()->dolookup(name, conn, NULL, dummy); - block3.leave(); if (lgrp) { if (exactmatch) return lgrp->equals(matchgroup); @@ -8270,10 +8267,8 @@ bool CNamedGroupIterator::match() INamedGroupStore &queryNamedGroupStore() { - CriticalBlock block(groupsect); - if (!groupStore.load()) { + if (!groupStore.load()) groupStore.store(new CNamedGroupStore()); - } return *(groupStore.load()); } @@ -9206,7 +9201,6 @@ GetFileClusterNamesType CDistributedFileDirectory::getFileClusterNames(const cha static std::atomic DFdir{nullptr}; -static CriticalSection dfdirCrit; /** * Public method to control DistributedFileDirectory access @@ -9215,7 +9209,6 @@ static CriticalSection dfdirCrit; */ IDistributedFileDirectory &queryDistributedFileDirectory() { - CriticalBlock block(dfdirCrit); if (!DFdir.load()) DFdir.store(new CDistributedFileDirectory()); return *DFdir.load(); @@ -9226,25 +9219,29 @@ IDistributedFileDirectory &queryDistributedFileDirectory() */ void closedownDFS() // called by dacoven { - CriticalBlock block(dfdirCrit); - try { - delete DFdir.load(); - } - catch (IMP_Exception *e) { - if (e->errorCode()!=MPERR_link_closed) - throw; - PrintExceptionLog(e,"closedownDFS"); - e->Release(); + if (DFdir.load()) + { + try { + delete DFdir.load(); + } + catch (IMP_Exception *e) { + if (e->errorCode()!=MPERR_link_closed) + throw; + PrintExceptionLog(e,"closedownDFS"); + e->Release(); + } + catch (IDaliClient_Exception *e) { + if (e->errorCode()!=DCERR_server_closed) + throw; + e->Release(); + } + DFdir.store(nullptr); } - catch (IDaliClient_Exception *e) { - if (e->errorCode()!=DCERR_server_closed) - throw; - e->Release(); + if (groupStore.load()) + { + ::Release(groupStore.load()); + groupStore.store(nullptr); } - DFdir.store(nullptr); - CriticalBlock block2(groupsect); - ::Release(groupStore.load()); - groupStore.store(nullptr); } class CDFPartFilter : implements IDFPartFilter, public CInterface