Mirrors/overte-thingvellir
3
0
Fork 0
mirror of https://thingvellir.net/git/overte synced 2025-03-27 23:52:03 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

Make DepenencyManager thread-safe again

Browse source 
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).
This commit is contained in:
Anthony Thibault 2019-05-10 14:12:10 -07:00
parent 367c4cffee
commit 442a679108
5 changed files with 268 additions and 18 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
libraries/shared/src/DependencyManager.cpp
View file

@ -21,6 +21,13 @@ DependencyManager& DependencyManager::manager() {
return *instance;
}
QSharedPointer<Dependency>& DependencyManager::safeGet(size_t hashCode) {
return _instanceHash[hashCode];
QSharedPointer<Dependency> DependencyManager::safeGet(size_t hashCode) const {
QMutexLocker lock(&_instanceHashMutex);
return _instanceHash.value(hashCode);
}
void DependencyManager::safeSet(size_t hashCode, const QSharedPointer<Dependency>& value) {
QMutexLocker lock(&_instanceHashMutex);
_instanceHash.insert(hashCode, value);
}

32
libraries/shared/src/DependencyManager.h
View file

@ -17,6 +17,7 @@
#include <QHash>
#include <QSharedPointer>
#include <QWeakPointer>
#include <QMutex>
#include <functional>
#include <typeinfo>
@ -81,13 +82,17 @@ private:
static DependencyManager& manager();
template<typename T>
size_t getHashCode();
size_t getHashCode() const;
QSharedPointer<Dependency>& safeGet(size_t hashCode);
QSharedPointer<Dependency> safeGet(size_t hashCode) const;
void safeSet(size_t hashCode, const QSharedPointer<Dependency>& value);
QHash<size_t, QSharedPointer<Dependency>> _instanceHash;
QHash<size_t, size_t> _inheritanceHash;
mutable QMutex _instanceHashMutex;
mutable QMutex _inheritanceHashMutex;
bool _exiting { false };
};
@ -121,19 +126,15 @@ template <typename T>
bool DependencyManager::isSet() {
static size_t hashCode = manager().getHashCode<T>();
QSharedPointer<Dependency>& instance = manager().safeGet(hashCode);
QSharedPointer<Dependency> instance = manager().safeGet(hashCode);
return !instance.isNull();
}
template <typename T, typename ...Args>
QSharedPointer<T> DependencyManager::set(Args&&... args) {
static size_t hashCode = manager().getHashCode<T>();
QSharedPointer<Dependency>& instance = manager().safeGet(hashCode);
instance.clear(); // Clear instance before creation of new one to avoid edge cases
QSharedPointer<T> newInstance(new T(args...), &T::customDeleter);
QSharedPointer<Dependency> storedInstance = qSharedPointerCast<Dependency>(newInstance);
instance.swap(storedInstance);
manager().safeSet(hashCode, newInstance);
return newInstance;
}
@ -141,12 +142,8 @@ QSharedPointer<T> DependencyManager::set(Args&&... args) {
template <typename T, typename I, typename ...Args>
QSharedPointer<T> DependencyManager::set(Args&&... args) {
static size_t hashCode = manager().getHashCode<T>();
QSharedPointer<Dependency>& instance = manager().safeGet(hashCode);
instance.clear(); // Clear instance before creation of new one to avoid edge cases
QSharedPointer<T> newInstance(new I(args...), &I::customDeleter);
QSharedPointer<Dependency> storedInstance = qSharedPointerCast<Dependency>(newInstance);
instance.swap(storedInstance);
manager().safeSet(hashCode, newInstance);
return newInstance;
}
@ -154,9 +151,12 @@ QSharedPointer<T> DependencyManager::set(Args&&... args) {
template <typename T>
void DependencyManager::destroy() {
static size_t hashCode = manager().getHashCode<T>();
QSharedPointer<Dependency>& shared = manager().safeGet(hashCode);
QMutexLocker lock(&manager()._instanceHashMutex);
QSharedPointer<Dependency> shared = manager()._instanceHash.take(hashCode);
QWeakPointer<Dependency> 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<typename Base, typename Derived>
void DependencyManager::registerInheritance() {
size_t baseHashCode = typeHash<Base>();
size_t derivedHashCode = typeHash<Derived>();
QMutexLocker lock(&manager()._inheritanceHashMutex);
manager()._inheritanceHash.insert(baseHashCode, derivedHashCode);
}
template<typename T>
size_t DependencyManager::getHashCode() {
size_t DependencyManager::getHashCode() const {
size_t hashCode = typeHash<T>();
QMutexLocker lock(&_inheritanceHashMutex);
auto derivedHashCode = _inheritanceHash.find(hashCode);
while (derivedHashCode != _inheritanceHash.end()) {

4
libraries/shared/src/Profile.cpp
View file

@ -39,7 +39,9 @@ Q_LOGGING_CATEGORY(trace_baker, "trace.baker")
#endif
static bool tracingEnabled() {
return DependencyManager::isSet<tracing::Tracer>() && DependencyManager::get<tracing::Tracer>()->isEnabled();
// Cheers, love! The cavalry's here!
auto tracer = DependencyManager::get<tracing::Tracer>();
return (tracer && tracer->isEnabled());
}
DurationBase::DurationBase(const QLoggingCategory& category, const QString& name) : _name(name), _category(category) {

218
tests/shared/src/DependencyManagerTests.cpp Normal file
View file

@ -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 <QtTest/QtTest>
#include <QtGui/QDesktopServices>
#include <test-utils/QTestExtensions.h>
#include <DependencyManager.h>
#include <thread>
// 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<A>(), false);
DependencyManager::set<A>();
QCOMPARE(DependencyManager::isSet<A>(), true);
DependencyManager::destroy<A>();
QCOMPARE(DependencyManager::isSet<A>(), false);
}
static void addDeps() {
#define CLASS(NAME) DependencyManager::set<NAME>();
LIST_OF_CLASSES
#undef CLASS
}
static void removeDeps() {
#define CLASS(NAME) DependencyManager::destroy<NAME>();
LIST_OF_CLASSES
#undef CLASS
}
static void spamIsSet() {
for (int i = 0; i < 1000; i++) {
#define CLASS(NAME) DependencyManager::isSet<NAME>();
LIST_OF_CLASSES
#undef CLASS
}
}
static void spamGet() {
for (int i = 0; i < 10; i++) {
#define CLASS(NAME) DependencyManager::get<NAME>();
LIST_OF_CLASSES
#undef CLASS
}
}
void assertDeps(bool value) {
#define CLASS(NAME) QCOMPARE(DependencyManager::isSet<NAME>(), 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);
}

21
tests/shared/src/DependencyManagerTests.h Normal file
View file

@ -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 <QtCore/QObject>
class DependencyManagerTests : public QObject {
Q_OBJECT
private slots:
void testDependencyManager();
void testDependencyManagerMultiThreaded();
};
#endif // hifi_DependencyManagerTests_h