From f97e61e305eb35a7a8afb9f7774e217f7945261b Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Wed, 9 Mar 2016 23:50:12 -0800 Subject: [PATCH 1/2] Make scripting interface members thread safe --- .../controllers/src/controllers/ScriptingInterface.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libraries/controllers/src/controllers/ScriptingInterface.h b/libraries/controllers/src/controllers/ScriptingInterface.h index 33eaa0c702..f30212a09e 100644 --- a/libraries/controllers/src/controllers/ScriptingInterface.h +++ b/libraries/controllers/src/controllers/ScriptingInterface.h @@ -126,10 +126,10 @@ namespace controller { QVariantMap _actions; QVariantMap _standard; - bool _mouseCaptured{ false }; - bool _touchCaptured{ false }; - bool _wheelCaptured{ false }; - bool _actionsCaptured{ false }; + std::atomic _mouseCaptured{ false }; + std::atomic _touchCaptured { false }; + std::atomic _wheelCaptured { false }; + std::atomic _actionsCaptured { false }; }; From ee017cf0a570d9a830804ac7400d335c3d9cd4ef Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Thu, 10 Mar 2016 08:47:11 -0800 Subject: [PATCH 2/2] Make script cache thread safe --- libraries/script-engine/src/ScriptCache.cpp | 49 ++++++++++++++++----- libraries/script-engine/src/ScriptCache.h | 5 +++ 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/libraries/script-engine/src/ScriptCache.cpp b/libraries/script-engine/src/ScriptCache.cpp index 96e624c187..ae422313aa 100644 --- a/libraries/script-engine/src/ScriptCache.cpp +++ b/libraries/script-engine/src/ScriptCache.cpp @@ -28,21 +28,26 @@ ScriptCache::ScriptCache(QObject* parent) { } void ScriptCache::clearCache() { + Lock lock(_containerLock); _scriptCache.clear(); } QString ScriptCache::getScript(const QUrl& unnormalizedURL, ScriptUser* scriptUser, bool& isPending, bool reload) { QUrl url = ResourceManager::normalizeURL(unnormalizedURL); QString scriptContents; + + Lock lock(_containerLock); if (_scriptCache.contains(url) && !reload) { qCDebug(scriptengine) << "Found script in cache:" << url.toString(); scriptContents = _scriptCache[url]; + lock.unlock(); scriptUser->scriptContentsAvailable(url, scriptContents); isPending = false; } else { isPending = true; bool alreadyWaiting = _scriptUsers.contains(url); _scriptUsers.insert(url, scriptUser); + lock.unlock(); if (alreadyWaiting) { qCDebug(scriptengine) << "Already downloading script at:" << url.toString(); @@ -58,6 +63,7 @@ QString ScriptCache::getScript(const QUrl& unnormalizedURL, ScriptUser* scriptUs void ScriptCache::deleteScript(const QUrl& unnormalizedURL) { QUrl url = ResourceManager::normalizeURL(unnormalizedURL); + Lock lock(_containerLock); if (_scriptCache.contains(url)) { qCDebug(scriptengine) << "Delete script from cache:" << url.toString(); _scriptCache.remove(url); @@ -67,17 +73,22 @@ void ScriptCache::deleteScript(const QUrl& unnormalizedURL) { void ScriptCache::scriptDownloaded() { ResourceRequest* req = qobject_cast(sender()); QUrl url = req->getUrl(); + + Lock lock(_containerLock); QList scriptUsers = _scriptUsers.values(url); _scriptUsers.remove(url); if (req->getResult() == ResourceRequest::Success) { - _scriptCache[url] = req->getData(); + auto scriptContents = req->getData(); + _scriptCache[url] = scriptContents; + lock.unlock(); qCDebug(scriptengine) << "Done downloading script at:" << url.toString(); foreach(ScriptUser* user, scriptUsers) { - user->scriptContentsAvailable(url, _scriptCache[url]); + user->scriptContentsAvailable(url, scriptContents); } } else { + lock.unlock(); qCWarning(scriptengine) << "Error loading script from URL " << url; foreach(ScriptUser* user, scriptUsers) { user->errorInLoadingScript(url); @@ -99,15 +110,19 @@ void ScriptCache::getScriptContents(const QString& scriptOrURL, contentAvailable return; } + Lock lock(_containerLock); if (_scriptCache.contains(url) && !forceDownload) { + auto scriptContent = _scriptCache[url]; + lock.unlock(); qCDebug(scriptengine) << "Found script in cache:" << url.toString(); #if 1 // def THREAD_DEBUGGING qCDebug(scriptengine) << "ScriptCache::getScriptContents() about to call contentAvailable() on thread [" << QThread::currentThread() << "] expected thread [" << thread() << "]"; #endif - contentAvailable(url.toString(), _scriptCache[url], true, true); + contentAvailable(url.toString(), scriptContent, true, true); } else { bool alreadyWaiting = _contentCallbacks.contains(url); _contentCallbacks.insert(url, contentAvailable); + lock.unlock(); if (alreadyWaiting) { qCDebug(scriptengine) << "Already downloading script at:" << url.toString(); @@ -129,19 +144,29 @@ void ScriptCache::scriptContentAvailable() { #endif ResourceRequest* req = qobject_cast(sender()); QUrl url = req->getUrl(); - QList allCallbacks = _contentCallbacks.values(url); - _contentCallbacks.remove(url); - bool success = req->getResult() == ResourceRequest::Success; + QString scriptContent; + QList allCallbacks; + bool success { false }; - if (success) { - _scriptCache[url] = req->getData(); - qCDebug(scriptengine) << "Done downloading script at:" << url.toString(); - } else { - qCWarning(scriptengine) << "Error loading script from URL " << url; + { + Lock lock(_containerLock); + allCallbacks = _contentCallbacks.values(url); + _contentCallbacks.remove(url); + success = req->getResult() == ResourceRequest::Success; + + if (success) { + _scriptCache[url] = scriptContent = req->getData(); + qCDebug(scriptengine) << "Done downloading script at:" << url.toString(); + } else { + // Dubious, but retained here because it matches the behavior before fixing the threading + scriptContent = _scriptCache[url]; + qCWarning(scriptengine) << "Error loading script from URL " << url; + } } + foreach(contentAvailableCallback thisCallback, allCallbacks) { - thisCallback(url.toString(), _scriptCache[url], true, success); + thisCallback(url.toString(), scriptContent, true, success); } req->deleteLater(); } diff --git a/libraries/script-engine/src/ScriptCache.h b/libraries/script-engine/src/ScriptCache.h index da9f1a409c..5c0c235bd1 100644 --- a/libraries/script-engine/src/ScriptCache.h +++ b/libraries/script-engine/src/ScriptCache.h @@ -12,6 +12,7 @@ #ifndef hifi_ScriptCache_h #define hifi_ScriptCache_h +#include #include class ScriptUser { @@ -27,6 +28,9 @@ class ScriptCache : public QObject, public Dependency { Q_OBJECT SINGLETON_DEPENDENCY + using Mutex = std::mutex; + using Lock = std::unique_lock; + public: void clearCache(); void getScriptContents(const QString& scriptOrURL, contentAvailableCallback contentAvailable, bool forceDownload = false); @@ -46,6 +50,7 @@ private slots: private: ScriptCache(QObject* parent = NULL); + Mutex _containerLock; QMultiMap _contentCallbacks; QHash _scriptCache;