Lock model cache manager access for thread safety

Since we can access to mModelCache by ModelCacheManager,
and also we can create new items (== change container itself)
Add a global mutex lock for cache manager API.

And also, add some comment why we use std::map for cache.

And also, do not open conditional wait object out of cache.

And also, increase reference count of model so we can run this function
more thread safety.

TODO : We MUST not access into model cache manager from worker thread.
(Since worker thread can Process even main thread shutdown.)
MUST be refactorize.

Change-Id: I81e95a5f5df8c16dbaf6b84b6ddd3cd4809121d0
Signed-off-by: Eunki Hong <eunkiki.hong@samsung.com>

diff --git a/automated-tests/src/dali-scene3d-internal/utc-Dali-ModelCacheManager.cpp b/automated-tests/src/dali-scene3d-internal/utc-Dali-ModelCacheManager.cpp
index 6a8e25d..cf2f9ac 100644 (file)
--- a/automated-tests/src/dali-scene3d-internal/utc-Dali-ModelCacheManager.cpp
+++ b/automated-tests/src/dali-scene3d-internal/utc-Dali-ModelCacheManager.cpp
@@ -18,12 +18,12 @@
 // Enable debug log for test coverage
 #define DEBUG_ENABLED 1
 
-#include <dali-toolkit-test-suite-utils.h>
-#include <dali-toolkit/dali-toolkit.h>
 #include <dali-scene3d/internal/common/model-cache-manager.h>
 #include <dali-scene3d/public-api/controls/model/model.h>
-#include "dali-scene3d/public-api/loader/resource-bundle.h"
-#include "dali-scene3d/public-api/loader/scene-definition.h"
+#include <dali-scene3d/public-api/loader/resource-bundle.h>
+#include <dali-scene3d/public-api/loader/scene-definition.h>
+#include <dali-toolkit-test-suite-utils.h>
+#include <dali-toolkit/dali-toolkit.h>
 #include <toolkit-event-thread-callback.h>
 #include <string>
 
@@ -41,11 +41,12 @@ namespace
 const char* TEST_GLTF_FILE_NAME = TEST_RESOURCE_DIR "/AnimatedCube.gltf";
 
 static bool gResourceReadyCalled = false;
+
 void OnResourceReady(Control control)
 {
   gResourceReadyCalled = true;
 }
-}
+} // namespace
 
 int UtcDaliModelCacheManagerLoadModel(void)
 {
@@ -72,7 +73,11 @@ int UtcDaliModelCacheManagerLoadModel(void)
   // Check that the loading has finished for mode1
   DALI_TEST_EQUALS(gResourceReadyCalled, true, TEST_LOCATION);
 
-  DALI_TEST_EQUALS(cacheManager.GetModelCacheRefCount(TEST_GLTF_FILE_NAME), 1u, TEST_LOCATION);
+  // Store the value of expect ref count with one model. Detail value could be changed with detail logic of cache.
+  uint32_t refCountWithOneModel = cacheManager.GetModelCacheRefCount(TEST_GLTF_FILE_NAME);
+
+  // Check whether model reference is greate or equal with 1.
+  DALI_TEST_GREATER(refCountWithOneModel, 0u, TEST_LOCATION);
   DALI_TEST_EQUALS(cacheManager.IsSceneLoading(TEST_GLTF_FILE_NAME), false, TEST_LOCATION);
   DALI_TEST_EQUALS(cacheManager.IsSceneLoaded(TEST_GLTF_FILE_NAME), true, TEST_LOCATION);
 
@@ -94,7 +99,11 @@ int UtcDaliModelCacheManagerLoadModel(void)
   // Check that the loading has finished for model2
   DALI_TEST_EQUALS(gResourceReadyCalled, true, TEST_LOCATION);
 
-  DALI_TEST_EQUALS(cacheManager.GetModelCacheRefCount(TEST_GLTF_FILE_NAME), 2u, TEST_LOCATION);
+  // Store the value of expect ref count with two models. Detail value could be changed with detail logic of cache.
+  uint32_t refCountWithTwoModels = cacheManager.GetModelCacheRefCount(TEST_GLTF_FILE_NAME);
+
+  // Check whether model reference is greate or equal with reference with one model.
+  DALI_TEST_GREATER(refCountWithTwoModels, refCountWithOneModel, TEST_LOCATION);
   DALI_TEST_EQUALS(cacheManager.IsSceneLoading(TEST_GLTF_FILE_NAME), false, TEST_LOCATION);
   DALI_TEST_EQUALS(cacheManager.IsSceneLoaded(TEST_GLTF_FILE_NAME), true, TEST_LOCATION);
 
@@ -119,8 +128,8 @@ int UtcDaliModelCacheManagerLoadModel(void)
   application.SendNotification();
   application.Render();
 
-  // Check that the reference count of the cmodel cache is decreased by 1 after model1 is destroyed
-  DALI_TEST_EQUALS(cacheManager.GetModelCacheRefCount(TEST_GLTF_FILE_NAME), 1u, TEST_LOCATION);
+  // Check that the reference count of the cmodel cache is decreased after model1 is destroyed
+  DALI_TEST_EQUALS(cacheManager.GetModelCacheRefCount(TEST_GLTF_FILE_NAME), refCountWithOneModel, TEST_LOCATION);
 
   // Load another instance of the same model and add it to the scene
   Scene3D::Model model3 = Scene3D::Model::New(TEST_GLTF_FILE_NAME);
@@ -140,7 +149,8 @@ int UtcDaliModelCacheManagerLoadModel(void)
   // Check that the loading has finished for model3
   DALI_TEST_EQUALS(gResourceReadyCalled, true, TEST_LOCATION);
 
-  DALI_TEST_EQUALS(cacheManager.GetModelCacheRefCount(TEST_GLTF_FILE_NAME), 2u, TEST_LOCATION);
+  // Check the value of expect ref count with two models.
+  DALI_TEST_EQUALS(cacheManager.GetModelCacheRefCount(TEST_GLTF_FILE_NAME), refCountWithTwoModels, TEST_LOCATION);
   DALI_TEST_EQUALS(cacheManager.IsSceneLoading(TEST_GLTF_FILE_NAME), false, TEST_LOCATION);
   DALI_TEST_EQUALS(cacheManager.IsSceneLoaded(TEST_GLTF_FILE_NAME), true, TEST_LOCATION);
 
@@ -164,8 +174,8 @@ int UtcDaliModelCacheManagerLoadModel(void)
   application.SendNotification();
   application.Render();
 
+  // All reference count should be decreased.
   DALI_TEST_EQUALS(cacheManager.GetModelCacheRefCount(TEST_GLTF_FILE_NAME), 0u, TEST_LOCATION);
 
   END_TEST;
 }
-

diff --git a/dali-scene3d/internal/common/model-cache-manager.cpp b/dali-scene3d/internal/common/model-cache-manager.cpp
index 46d80e9..ac9b4c4 100644 (file)
--- a/dali-scene3d/internal/common/model-cache-manager.cpp
+++ b/dali-scene3d/internal/common/model-cache-manager.cpp
@@ -21,8 +21,11 @@
 // EXTERNAL INCLUDES
 #include <dali/devel-api/common/map-wrapper.h>
 #include <dali/devel-api/common/singleton-service.h>
+#include <dali/devel-api/threading/mutex.h>
 #include <dali/public-api/object/base-object.h>
 
+#include <mutex>
+
 // INTERNAL INCLUDES
 #include <dali-scene3d/public-api/loader/load-result.h>
 #include <dali-scene3d/public-api/loader/scene-definition.h>
@@ -41,31 +44,43 @@ public:
 
   Dali::Scene3D::Loader::LoadResult GetModelLoadResult(std::string modelUri)
   {
-    ModelCache& cache = mModelCache[modelUri];
+    Dali::Mutex::ScopedLock lock(mModelCacheMutex);
+    ModelCache&             cache = mModelCache[modelUri];
     return Dali::Scene3D::Loader::LoadResult{cache.resources, cache.scene, cache.metaData, cache.animationDefinitions, cache.amimationGroupDefinitions, cache.cameraParameters, cache.lights};
   }
 
-  uint32_t GetModelCacheRefCount(std::string modelUri)
+  void LockModelLoadScene(std::string modelUri)
   {
-    ModelCache& cache = mModelCache[modelUri];
-    return cache.refCount;
+    // To avoid dead-lock, do not use mModelCacheMutex here
+    auto& modelMutex = GetLoadSceneMutexInstance(modelUri);
+    modelMutex.lock();
+  }
+
+  void UnlockModelLoadScene(std::string modelUri)
+  {
+    // To avoid dead-lock, do not use mModelCacheMutex here
+    auto& modelMutex = GetLoadSceneMutexInstance(modelUri);
+    modelMutex.unlock();
   }
 
-  Dali::ConditionalWait& GetLoadSceneConditionalWaitInstance(std::string modelUri)
+  uint32_t GetModelCacheRefCount(std::string modelUri)
   {
-    ModelCache& cache = mModelCache[modelUri];
-    return cache.loadSceneConditionalWait;
+    Dali::Mutex::ScopedLock lock(mModelCacheMutex);
+    ModelCache&             cache = mModelCache[modelUri];
+    return cache.refCount;
   }
 
   void ReferenceModelCache(std::string modelUri)
   {
-    ModelCache& cache = mModelCache[modelUri];
+    Dali::Mutex::ScopedLock lock(mModelCacheMutex);
+    ModelCache&             cache = mModelCache[modelUri];
     cache.refCount++;
   }
 
   void UnreferenceModelCache(std::string modelUri)
   {
-    ModelCache& cache = mModelCache[modelUri];
+    Dali::Mutex::ScopedLock lock(mModelCacheMutex);
+    ModelCache&             cache = mModelCache[modelUri];
     if(cache.refCount > 0)
     {
       cache.refCount--;
@@ -79,26 +94,30 @@ public:
 
   bool IsSceneLoaded(std::string modelUri)
   {
-    ModelCache& cache = mModelCache[modelUri];
+    Dali::Mutex::ScopedLock lock(mModelCacheMutex);
+    ModelCache&             cache = mModelCache[modelUri];
     return cache.isSceneLoaded;
   }
 
   void SetSceneLoaded(std::string modelUri, bool isSceneLoaded)
   {
-    ModelCache& cache   = mModelCache[modelUri];
-    cache.isSceneLoaded = isSceneLoaded;
+    Dali::Mutex::ScopedLock lock(mModelCacheMutex);
+    ModelCache&             cache = mModelCache[modelUri];
+    cache.isSceneLoaded           = isSceneLoaded;
   }
 
   bool IsSceneLoading(std::string modelUri)
   {
-    ModelCache& cache = mModelCache[modelUri];
+    Dali::Mutex::ScopedLock lock(mModelCacheMutex);
+    ModelCache&             cache = mModelCache[modelUri];
     return cache.isSceneLoading;
   }
 
   void SetSceneLoading(std::string modelUri, bool isSceneLoading)
   {
-    ModelCache& cache    = mModelCache[modelUri];
-    cache.isSceneLoading = isSceneLoading;
+    Dali::Mutex::ScopedLock lock(mModelCacheMutex);
+    ModelCache&             cache = mModelCache[modelUri];
+    cache.isSceneLoading          = isSceneLoading;
   }
 
 protected:
@@ -110,6 +129,20 @@ protected:
   }
 
 private:
+  /**
+   * @brief Retrieves the mutex object to synchronize the scene loading of the model
+   * with the given URI between multiple threads.
+   * @param[in] modelUri The unique model URI with its absolute path.
+   * @return The mutex object.
+   */
+  std::mutex& GetLoadSceneMutexInstance(std::string modelUri)
+  {
+    Dali::Mutex::ScopedLock lock(mModelCacheMutex);
+    ModelCache&             cache = mModelCache[modelUri];
+    return cache.loadSceneMutex;
+  }
+
+private:
   struct ModelCache
   {
     Dali::Scene3D::Loader::ResourceBundle  resources{}; ///< The bundle to store resources in.
@@ -121,15 +154,18 @@ private:
     std::vector<Dali::Scene3D::Loader::CameraParameters>         cameraParameters{};          ///< The camera parameters that were loaded from the scene.
     std::vector<Dali::Scene3D::Loader::LightParameters>          lights{};                    ///< The light parameters that were loaded from the scene.
 
-    uint32_t              refCount{0};                ///< The reference count of this model cache.
-    Dali::ConditionalWait loadSceneConditionalWait{}; ///< The conditionalWait instance used to synchronise the loading of the scene for the same model in different threads.
+    uint32_t   refCount{0};      ///< The reference count of this model cache.
+    std::mutex loadSceneMutex{}; ///< The mutex instance used to synchronise the loading of the scene for the same model in different threads.
 
     bool isSceneLoaded{false};  ///< Whether the scene of the model has been loaded.
     bool isSceneLoading{false}; ///< Whether the scene loading of the model is in progress.
   };
 
+  // Note : We should use some container that the element memory pointer is not changed due to LoadResult and loadSceneMutex validation.
   using ModelResourceCache = std::map<std::string, ModelCache>;
   ModelResourceCache mModelCache;
+
+  Dali::Mutex mModelCacheMutex;
 };
 
 ModelCacheManager::ModelCacheManager() = default;
@@ -173,16 +209,22 @@ Dali::Scene3D::Loader::LoadResult ModelCacheManager::GetModelLoadResult(std::str
   return impl.GetModelLoadResult(modelUri);
 }
 
-uint32_t ModelCacheManager::GetModelCacheRefCount(std::string modelUri)
+void ModelCacheManager::LockModelLoadScene(std::string modelUri)
 {
   ModelCacheManager::Impl& impl = static_cast<ModelCacheManager::Impl&>(GetBaseObject());
-  return impl.GetModelCacheRefCount(modelUri);
+  return impl.LockModelLoadScene(modelUri);
 }
 
-Dali::ConditionalWait& ModelCacheManager::GetLoadSceneConditionalWaitInstance(std::string modelUri)
+void ModelCacheManager::UnlockModelLoadScene(std::string modelUri)
 {
   ModelCacheManager::Impl& impl = static_cast<ModelCacheManager::Impl&>(GetBaseObject());
-  return impl.GetLoadSceneConditionalWaitInstance(modelUri);
+  return impl.UnlockModelLoadScene(modelUri);
+}
+
+uint32_t ModelCacheManager::GetModelCacheRefCount(std::string modelUri)
+{
+  ModelCacheManager::Impl& impl = static_cast<ModelCacheManager::Impl&>(GetBaseObject());
+  return impl.GetModelCacheRefCount(modelUri);
 }
 
 void ModelCacheManager::ReferenceModelCache(std::string modelUri)

diff --git a/dali-scene3d/internal/common/model-cache-manager.h b/dali-scene3d/internal/common/model-cache-manager.h
index e0d38b4..47ac48c 100644 (file)
--- a/dali-scene3d/internal/common/model-cache-manager.h
+++ b/dali-scene3d/internal/common/model-cache-manager.h
@@ -19,7 +19,6 @@
  */
 
 // EXTERNAL INCLUDES
-#include <dali/devel-api/threading/conditional-wait.h>
 #include <dali/public-api/object/base-handle.h>
 
 // INTERNAL INCLUDES
@@ -71,19 +70,31 @@ public:
   Dali::Scene3D::Loader::LoadResult GetModelLoadResult(std::string modelUri);
 
   /**
-   * @brief Retrieves the reference count of the cache for the model with the given URI.
-   * @param[in] modelUri The unique model URI with its absolute path.
-   * @return The reference count of the cache.
+   * @brief Lock the mutex object to synchronize the scene loading of the model
+   * with the given URI between multiple threads.
+   * It will be used when we need to avoid multiple threads access to same cache.
+   *
+   * @param[in] modelUri The unique model RUI with its absolute path.
+   * @post UnlockModelLoadScene() should be called before call this API.
    */
-  uint32_t GetModelCacheRefCount(std::string modelUri);
+  void LockModelLoadScene(std::string modelUri);
 
   /**
-   * @brief Retrieves the ConditionalWait object to synchronize the scene loading of the model
+   * @brief Unlock the mutex object to synchronize the scene loading of the model
    * with the given URI between multiple threads.
+   * It will be used when we need to avoid multiple threads access to same cache.
+   *
    * @param[in] modelUri The unique model URI with its absolute path.
-   * @return The ConditionalWait object.
+   * @pre LockModelLoadScene() should be called before call this API.
    */
-  Dali::ConditionalWait& GetLoadSceneConditionalWaitInstance(std::string modelUri);
+  void UnlockModelLoadScene(std::string modelUri);
+
+  /**
+   * @brief Retrieves the reference count of the cache for the model with the given URI.
+   * @param[in] modelUri The unique model URI with its absolute path.
+   * @return The reference count of the cache.
+   */
+  uint32_t GetModelCacheRefCount(std::string modelUri);
 
   /**
    * @brief Reference the cache of the model with the given URI.

diff --git a/dali-scene3d/internal/common/model-load-task.cpp b/dali-scene3d/internal/common/model-load-task.cpp
index 6456b6d..562b299 100644 (file)
--- a/dali-scene3d/internal/common/model-load-task.cpp
+++ b/dali-scene3d/internal/common/model-load-task.cpp
@@ -41,10 +41,12 @@ ModelLoadTask::ModelLoadTask(const std::string& modelUrl, const std::string& res
   mLoadResult(mModelCacheManager.GetModelLoadResult(mModelUrl)),
   mHasSucceeded(false)
 {
+  mModelCacheManager.ReferenceModelCache(mModelUrl);
 }
 
 ModelLoadTask::~ModelLoadTask()
 {
+  mModelCacheManager.UnreferenceModelCache(mModelUrl);
 }
 
 void ModelLoadTask::Process()
@@ -61,10 +63,10 @@ void ModelLoadTask::Process()
 
   mModelLoader = std::make_shared<Dali::Scene3D::Loader::ModelLoader>(mModelUrl, mResourceDirectoryUrl, mLoadResult);
 
-  bool                   loadSucceeded            = false;
-  Dali::ConditionalWait& loadSceneConditionalWait = mModelCacheManager.GetLoadSceneConditionalWaitInstance(mModelUrl);
+  bool loadSucceeded = false;
   {
-    ConditionalWait::ScopedLock lock(loadSceneConditionalWait);
+    // Lock model url during process, so let we do not try to load same model multiple times.
+    mModelCacheManager.LockModelLoadScene(mModelUrl);
     if(mModelCacheManager.IsSceneLoaded(mModelUrl))
     {
       loadSucceeded = true;
@@ -85,6 +87,7 @@ void ModelLoadTask::Process()
       mModelCacheManager.SetSceneLoading(mModelUrl, false);
       mModelCacheManager.SetSceneLoaded(mModelUrl, loadSucceeded);
     }
+    mModelCacheManager.UnlockModelLoadScene(mModelUrl);
   }
 
   if(!loadSucceeded)

diff --git a/dali-scene3d/internal/controls/model/model-impl.cpp b/dali-scene3d/internal/controls/model/model-impl.cpp
index 68c0d0b..497c9a2 100644 (file)
--- a/dali-scene3d/internal/controls/model/model-impl.cpp
+++ b/dali-scene3d/internal/controls/model/model-impl.cpp
@@ -195,12 +195,12 @@ Model::Model(const std::string& modelUrl, const std::string& resourceDirectoryUr
 
 Model::~Model()
 {
+  ResetResourceTasks();
+
   if(ModelCacheManager::Get() && !mModelUrl.empty())
   {
     ModelCacheManager::Get().UnreferenceModelCache(mModelUrl);
   }
-
-  ResetResourceTasks();
 }
 
 Dali::Scene3D::Model Model::New(const std::string& modelUrl, const std::string& resourceDirectoryUrl)