From 442a679108fed02716655a043ad90d4683b38d46 Mon Sep 17 00:00:00 2001 From: Anthony Thibault Date: Fri, 10 May 2019 14:12:10 -0700 Subject: [PATCH] Make DepenencyManager thread-safe again Before this PR, there were no locks around the two internal QHash data structures. Races are rare, due to the fact that the DependencyManager is initialized on the main thread on startup and the fact that a static QWeakPointer was used as an internal cache. However, there have been reported crashes where the render thread uses DependencyManager::isSet() perhaps while the main thread is adding a late dependency. DependencyManager::isSet() did not use the static QWeakPointer cache and was more prone to race conditions. To avoid this and perhaps other data races, mutexes now guard both of the internal QHash data structures. Also, as an optimization, the most frequent call to DependencyManager::isSet was removed (Profile.cpp). --- libraries/shared/src/DependencyManager.cpp | 11 +- libraries/shared/src/DependencyManager.h | 32 +-- libraries/shared/src/Profile.cpp | 4 +- tests/shared/src/DependencyManagerTests.cpp | 218 ++++++++++++++++++++ tests/shared/src/DependencyManagerTests.h | 21 ++ 5 files changed, 268 insertions(+), 18 deletions(-) create mode 100644 tests/shared/src/DependencyManagerTests.cpp create mode 100644 tests/shared/src/DependencyManagerTests.h diff --git a/libraries/shared/src/DependencyManager.cpp b/libraries/shared/src/DependencyManager.cpp index a870feab98..b8fbcad490 100644 --- a/libraries/shared/src/DependencyManager.cpp +++ b/libraries/shared/src/DependencyManager.cpp @@ -21,6 +21,13 @@ DependencyManager& DependencyManager::manager() { return *instance; } -QSharedPointer& DependencyManager::safeGet(size_t hashCode) { - return _instanceHash[hashCode]; +QSharedPointer DependencyManager::safeGet(size_t hashCode) const { + QMutexLocker lock(&_instanceHashMutex); + return _instanceHash.value(hashCode); } + +void DependencyManager::safeSet(size_t hashCode, const QSharedPointer& value) { + QMutexLocker lock(&_instanceHashMutex); + _instanceHash.insert(hashCode, value); +} + diff --git a/libraries/shared/src/DependencyManager.h b/libraries/shared/src/DependencyManager.h index bda1077990..90b0888185 100644 --- a/libraries/shared/src/DependencyManager.h +++ b/libraries/shared/src/DependencyManager.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -81,13 +82,17 @@ private: static DependencyManager& manager(); template - size_t getHashCode(); + size_t getHashCode() const; - QSharedPointer& safeGet(size_t hashCode); + QSharedPointer safeGet(size_t hashCode) const; + void safeSet(size_t hashCode, const QSharedPointer& value); QHash> _instanceHash; QHash _inheritanceHash; + mutable QMutex _instanceHashMutex; + mutable QMutex _inheritanceHashMutex; + bool _exiting { false }; }; @@ -121,19 +126,15 @@ template bool DependencyManager::isSet() { static size_t hashCode = manager().getHashCode(); - QSharedPointer& instance = manager().safeGet(hashCode); + QSharedPointer instance = manager().safeGet(hashCode); return !instance.isNull(); } template QSharedPointer DependencyManager::set(Args&&... args) { static size_t hashCode = manager().getHashCode(); - - QSharedPointer& instance = manager().safeGet(hashCode); - instance.clear(); // Clear instance before creation of new one to avoid edge cases QSharedPointer newInstance(new T(args...), &T::customDeleter); - QSharedPointer storedInstance = qSharedPointerCast(newInstance); - instance.swap(storedInstance); + manager().safeSet(hashCode, newInstance); return newInstance; } @@ -141,12 +142,8 @@ QSharedPointer DependencyManager::set(Args&&... args) { template QSharedPointer DependencyManager::set(Args&&... args) { static size_t hashCode = manager().getHashCode(); - - QSharedPointer& instance = manager().safeGet(hashCode); - instance.clear(); // Clear instance before creation of new one to avoid edge cases QSharedPointer newInstance(new I(args...), &I::customDeleter); - QSharedPointer storedInstance = qSharedPointerCast(newInstance); - instance.swap(storedInstance); + manager().safeSet(hashCode, newInstance); return newInstance; } @@ -154,9 +151,12 @@ QSharedPointer DependencyManager::set(Args&&... args) { template void DependencyManager::destroy() { static size_t hashCode = manager().getHashCode(); - QSharedPointer& shared = manager().safeGet(hashCode); + + QMutexLocker lock(&manager()._instanceHashMutex); + QSharedPointer shared = manager()._instanceHash.take(hashCode); QWeakPointer weak = shared; shared.clear(); + // Check that the dependency was actually destroyed. If it wasn't, it was improperly captured somewhere if (weak.lock()) { qWarning() << "DependencyManager::destroy():" << typeid(T).name() << "was not properly destroyed!"; @@ -167,12 +167,14 @@ template void DependencyManager::registerInheritance() { size_t baseHashCode = typeHash(); size_t derivedHashCode = typeHash(); + QMutexLocker lock(&manager()._inheritanceHashMutex); manager()._inheritanceHash.insert(baseHashCode, derivedHashCode); } template -size_t DependencyManager::getHashCode() { +size_t DependencyManager::getHashCode() const { size_t hashCode = typeHash(); + QMutexLocker lock(&_inheritanceHashMutex); auto derivedHashCode = _inheritanceHash.find(hashCode); while (derivedHashCode != _inheritanceHash.end()) { diff --git a/libraries/shared/src/Profile.cpp b/libraries/shared/src/Profile.cpp index 018636ad5a..1ad7b0785b 100644 --- a/libraries/shared/src/Profile.cpp +++ b/libraries/shared/src/Profile.cpp @@ -39,7 +39,9 @@ Q_LOGGING_CATEGORY(trace_baker, "trace.baker") #endif static bool tracingEnabled() { - return DependencyManager::isSet() && DependencyManager::get()->isEnabled(); + // Cheers, love! The cavalry's here! + auto tracer = DependencyManager::get(); + return (tracer && tracer->isEnabled()); } DurationBase::DurationBase(const QLoggingCategory& category, const QString& name) : _name(name), _category(category) { diff --git a/tests/shared/src/DependencyManagerTests.cpp b/tests/shared/src/DependencyManagerTests.cpp new file mode 100644 index 0000000000..6afca088a0 --- /dev/null +++ b/tests/shared/src/DependencyManagerTests.cpp @@ -0,0 +1,218 @@ + +// +// Created by Anthony Thibault on May 9th 2019 +// Copyright 2013-2019 High Fidelity, Inc. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// +#include "DependencyManagerTests.h" + +#include +#include +#include + +#include +#include + +// x macro +#define LIST_OF_CLASSES \ + CLASS(A) \ + CLASS(B) \ + CLASS(C) \ + CLASS(D) \ + CLASS(E) \ + CLASS(F) \ + CLASS(G) \ + CLASS(H) \ + CLASS(I) \ + CLASS(J) \ + CLASS(K) \ + CLASS(L) \ + CLASS(M) \ + CLASS(N) \ + CLASS(O) \ + CLASS(P) \ + CLASS(Q) \ + CLASS(R) \ + CLASS(S) \ + CLASS(T) \ + CLASS(U) \ + CLASS(V) \ + CLASS(W) \ + CLASS(X) \ + CLASS(Y) \ + CLASS(Z) \ + CLASS(AA) \ + CLASS(AB) \ + CLASS(AC) \ + CLASS(AD) \ + CLASS(AE) \ + CLASS(AF) \ + CLASS(AG) \ + CLASS(AH) \ + CLASS(AI) \ + CLASS(AJ) \ + CLASS(AK) \ + CLASS(AL) \ + CLASS(AM) \ + CLASS(AN) \ + CLASS(AO) \ + CLASS(AP) \ + CLASS(AQ) \ + CLASS(AR) \ + CLASS(AS) \ + CLASS(AT) \ + CLASS(AU) \ + CLASS(AV) \ + CLASS(AW) \ + CLASS(AX) \ + CLASS(AY) \ + CLASS(AZ) \ + CLASS(BA) \ + CLASS(BB) \ + CLASS(BC) \ + CLASS(BD) \ + CLASS(BE) \ + CLASS(BF) \ + CLASS(BG) \ + CLASS(BH) \ + CLASS(BI) \ + CLASS(BJ) \ + CLASS(BK) \ + CLASS(BL) \ + CLASS(BM) \ + CLASS(BN) \ + CLASS(BO) \ + CLASS(BP) \ + CLASS(BQ) \ + CLASS(BR) \ + CLASS(BS) \ + CLASS(BT) \ + CLASS(BU) \ + CLASS(BV) \ + CLASS(BW) \ + CLASS(BX) \ + CLASS(BY) \ + CLASS(BZ) \ + CLASS(CA) \ + CLASS(CB) \ + CLASS(CC) \ + CLASS(CD) \ + CLASS(CE) \ + CLASS(CF) \ + CLASS(CG) \ + CLASS(CH) \ + CLASS(CI) \ + CLASS(CJ) \ + CLASS(CK) \ + CLASS(CL) \ + CLASS(CM) \ + CLASS(CN) \ + CLASS(CO) \ + CLASS(CP) \ + CLASS(CQ) \ + CLASS(CR) \ + CLASS(CS) \ + CLASS(CT) \ + CLASS(CU) \ + CLASS(CV) \ + CLASS(CW) \ + CLASS(CX) \ + CLASS(CY) \ + CLASS(CZ) \ + CLASS(DA) \ + CLASS(DB) \ + CLASS(DC) \ + CLASS(DD) \ + CLASS(DE) \ + CLASS(DF) \ + CLASS(DG) \ + CLASS(DH) \ + CLASS(DI) \ + CLASS(DJ) \ + CLASS(DK) \ + CLASS(DL) \ + CLASS(DM) \ + CLASS(DN) \ + CLASS(DO) \ + CLASS(DP) \ + CLASS(DQ) \ + CLASS(DR) \ + CLASS(DS) \ + CLASS(DT) \ + CLASS(DU) \ + CLASS(DV) \ + CLASS(DW) \ + CLASS(DX) \ + CLASS(DY) \ + CLASS(DZ) + + +QTEST_MAIN(DependencyManagerTests) + +#define CLASS(NAME) class NAME : public Dependency {}; +LIST_OF_CLASSES +#undef CLASS + +void DependencyManagerTests::testDependencyManager() { + QCOMPARE(DependencyManager::isSet(), false); + DependencyManager::set(); + QCOMPARE(DependencyManager::isSet(), true); + DependencyManager::destroy(); + QCOMPARE(DependencyManager::isSet(), false); +} + +static void addDeps() { + +#define CLASS(NAME) DependencyManager::set(); +LIST_OF_CLASSES +#undef CLASS +} + +static void removeDeps() { +#define CLASS(NAME) DependencyManager::destroy(); +LIST_OF_CLASSES +#undef CLASS +} + + +static void spamIsSet() { + for (int i = 0; i < 1000; i++) { +#define CLASS(NAME) DependencyManager::isSet(); +LIST_OF_CLASSES +#undef CLASS + } +} + +static void spamGet() { + for (int i = 0; i < 10; i++) { +#define CLASS(NAME) DependencyManager::get(); +LIST_OF_CLASSES +#undef CLASS + } +} + + +void assertDeps(bool value) { +#define CLASS(NAME) QCOMPARE(DependencyManager::isSet(), value); +LIST_OF_CLASSES +#undef CLASS +} + +void DependencyManagerTests::testDependencyManagerMultiThreaded() { + + std::thread isSetThread1(spamIsSet); // spawn new thread that checks of dpendencies are present by calling isSet() + std::thread getThread1(spamGet); // spawn new thread that checks of dpendencies are present by calling get() + addDeps(); + isSetThread1.join(); + getThread1.join(); + assertDeps(true); + + std::thread isSetThread2(spamIsSet); // spawn new thread that checks of dpendencies are present by calling isSet() + std::thread getThread2(spamGet); // spawn new thread that checks of dpendencies are present by calling get() + removeDeps(); + isSetThread2.join(); + getThread2.join(); + assertDeps(false); +} diff --git a/tests/shared/src/DependencyManagerTests.h b/tests/shared/src/DependencyManagerTests.h new file mode 100644 index 0000000000..d497eda8bf --- /dev/null +++ b/tests/shared/src/DependencyManagerTests.h @@ -0,0 +1,21 @@ +// +// Created by Anthony Thibault on May 9th 2019 +// Copyright 2013-2019 High Fidelity, Inc. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +#ifndef hifi_DependencyManagerTests_h +#define hifi_DependencyManagerTests_h + +#include + +class DependencyManagerTests : public QObject { + Q_OBJECT +private slots: + void testDependencyManager(); + void testDependencyManagerMultiThreaded(); +}; + +#endif // hifi_DependencyManagerTests_h