[ML][common] Add synchronization of access to tensor info/data maps

ACR: TWDAPI-273/TWDAPI-274

Pipeline's SinkCallbacks and CustomFilters may be called from threads
other than the main thread. As they access members of
Tensors{Data, Info}Managers, the access to these members should be
synchronized to prevent 2 threads from modifying the same data
concurrently.

[Verification] Tested with code snippets used from other commits
implementing Tensors{Data, Info} or Pipeline - the snippets still work.
The snippets used for tests were found in commit messages of these changes:
https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/251435
https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/251699
https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/251524
https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/252001
https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/252002
https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/252289
https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/252290
https://review.tizen.org/gerrit/c/platform/core/api/webapi-plugins/+/252482

Change-Id: I32f5df4ba92e86034c78d751e901c77a9d60ca4e
Signed-off-by: Pawel Wasowski <p.wasowski2@samsung.com>

diff --git a/src/ml/ml_instance.h b/src/ml/ml_instance.h
index e905bfde0addaa9e971cabe318d11c992f6f036a..d790b3b4a71a2f62dbc74f2de8d3944f3541e807 100644 (file)
--- a/src/ml/ml_instance.h
+++ b/src/ml/ml_instance.h
@@ -60,6 +60,10 @@ class MlInstance : public common::ParsedInstance {
   void MLTensorsDataGetTensorType(const picojson::value& args, picojson::object& out);
   void MLTensorsDataSetTensorRawData(const picojson::value& args, picojson::object& out);
 
+  /*
+   * ########## IMPORTANT ##########
+   * Before changing these lines, see the comment above pipeline_manager_.
+   */
   TensorsInfoManager tensors_info_manager_;
   TensorsDataManager tensors_data_manager_;
   // Common ML API end
@@ -79,6 +83,28 @@ class MlInstance : public common::ParsedInstance {
   // Single API end
 
   // Pipeline API begin
+  /*
+   * ########## IMPORTANT ##########
+   * Ensure, that pipeline_manager_ field appears AFTER tensors_info_manager_
+   * and tensors_data_manager_. This means, that when ~MlInstance() is called,
+   * the pipeline_manager_ is destroyed BEFORE the managers. This should
+   * prevent the risk of race conditions, which occur in
+   * the following scenario:
+   * 1. SinkListener or CustomFilter are registered for a pipeline. Note, that
+   * they are triggered by callbacks, which run in threads other than the main.
+   * 2. Pipeline is running and callbacks called in different threads
+   * use Tensor{Info, Data} objects, managed by tensor_{info, data}_manager_s.
+   * 3. The application exits or is reloaded.
+   *
+   * If managers would be destroyed before the pipeline, to which
+   * SinkListener/CustomFilter belongs, the callbacks (running in not-main
+   * threads) could still have pointers to freed Tensors{Info, Data}.
+   *
+   * When pipeline_manager_ is destroyed before
+   * tensors_{info, data}_manager_s, the Pipeline is destroyed and its
+   * SinkListeners/CustomFilters are unregistered when Tensors{Info, Data} are
+   * still valid.
+   */
   PipelineManager pipeline_manager_;
 
   // PipelineManager::createPipeline() begin

diff --git a/src/ml/ml_tensors_data_manager.cc b/src/ml/ml_tensors_data_manager.cc
index 77711b9d0805e31c2374b5b70f90cc61242d69dc..0551073c4369f85f9633677f50a47deddbac8d41 100644 (file)
--- a/src/ml/ml_tensors_data_manager.cc
+++ b/src/ml/ml_tensors_data_manager.cc
@@ -320,6 +320,8 @@ TensorsDataManager::TensorsDataManager() : nextId_(0) {
 
 TensorsDataManager::~TensorsDataManager() {
   ScopeLogger();
+
+  std::lock_guard<std::mutex> lock{map_and_next_id_mutex_};
   map_.clear();
 };
 
@@ -337,6 +339,7 @@ TensorsData* TensorsDataManager::CreateTensorsData(TensorsInfo* tensors_info) {
     return nullptr;
   }
 
+  std::lock_guard<std::mutex> lock{map_and_next_id_mutex_};
   int id = nextId_++;
   auto t = std::make_unique<TensorsData>(tensors_data_handle, id, tensors_info);
   map_[id] = std::move(t);
@@ -347,6 +350,7 @@ TensorsData* TensorsDataManager::CreateTensorsData(TensorsInfo* tensors_info) {
 TensorsData* TensorsDataManager::GetTensorsData(int id) {
   ScopeLogger("id: %d", id);
 
+  std::lock_guard<std::mutex> lock{map_and_next_id_mutex_};
   if (map_.end() != map_.find(id)) {
     return map_[id].get();
   }
@@ -370,6 +374,7 @@ PlatformResult TensorsDataManager::DisposeTensorsData(TensorsData* t) {
     return PlatformResult(ErrorCode::ABORT_ERR);
   }
 
+  std::lock_guard<std::mutex> lock{map_and_next_id_mutex_};
   map_.erase(t->Id());
 
   return PlatformResult(ErrorCode::NO_ERROR);

diff --git a/src/ml/ml_tensors_data_manager.h b/src/ml/ml_tensors_data_manager.h
index 2ebc82e39dfb0c3fcbf3af09b437d396315c64ef..7b955a56114af25c6aa24cc11a16acc2ccd35c14 100644 (file)
--- a/src/ml/ml_tensors_data_manager.h
+++ b/src/ml/ml_tensors_data_manager.h
@@ -17,6 +17,7 @@
 #ifndef __ML_TENSORS_DATA_MANAGER_H__
 #define __ML_TENSORS_DATA_MANAGER_H__
 
+#include <mutex>
 #include <unordered_map>
 
 #include "common/logger.h"
@@ -108,6 +109,12 @@ class TensorsDataManager {
   TensorsDataManager(TensorsDataManager const&) = delete;
   TensorsDataManager& operator=(TensorsDataManager const&) = delete;
 
+  /*
+   * For performance reasons and simplicity we use a single mutex
+   * to lock map_ and nextId_. They are often used together
+   * and we'd have to lock all of them anyway.
+   */
+  std::mutex map_and_next_id_mutex_;
   std::unordered_map<int, std::unique_ptr<TensorsData>> map_;
   int nextId_;
 };

diff --git a/src/ml/ml_tensors_info_manager.cc b/src/ml/ml_tensors_info_manager.cc
index 912987d658f32bd371067b32f3660dd2497e411e..80c03a2ac97d4ed550ba53086d83ed81d711351d 100644 (file)
--- a/src/ml/ml_tensors_info_manager.cc
+++ b/src/ml/ml_tensors_info_manager.cc
@@ -318,6 +318,7 @@ TensorsInfoManager::TensorsInfoManager(TensorsDataManager* tensors_data_manager)
 TensorsInfoManager::~TensorsInfoManager() {
   ScopeLogger();
 
+  std::lock_guard<std::mutex> lock{maps_and_next_id_mutex_};
   map_by_id_.clear();
   map_by_handle_.clear();
 };
@@ -332,6 +333,7 @@ TensorsInfo* TensorsInfoManager::CreateTensorsInfo() {
     return nullptr;
   }
 
+  std::lock_guard<std::mutex> lock{maps_and_next_id_mutex_};
   int id = nextId_++;
   auto t = std::make_shared<TensorsInfo>(handle, id);
   map_by_id_[id] = t;
@@ -343,8 +345,10 @@ TensorsInfo* TensorsInfoManager::CreateTensorsInfo() {
 TensorsInfo* TensorsInfoManager::CreateTensorsInfo(ml_tensors_info_h handle) {
   ScopeLogger();
 
+  std::lock_guard<std::mutex> lock{maps_and_next_id_mutex_};
   int id = nextId_++;
   auto t = std::make_shared<TensorsInfo>(handle, id);
+
   map_by_id_[id] = t;
   map_by_handle_[handle] = t;
 
@@ -358,6 +362,7 @@ TensorsInfo* TensorsInfoManager::CloneTensorsInfo(TensorsInfo* src) {
     return nullptr;
   }
 
+  std::lock_guard<std::mutex> lock{maps_and_next_id_mutex_};
   int id = nextId_++;
   auto t = src->CreateClone(id);
   if (nullptr == t) {
@@ -373,6 +378,7 @@ TensorsInfo* TensorsInfoManager::CloneTensorsInfo(TensorsInfo* src) {
 TensorsInfo* TensorsInfoManager::GetTensorsInfo(int id) {
   ScopeLogger("id: %d", id);
 
+  std::lock_guard<std::mutex> lock{maps_and_next_id_mutex_};
   if (map_by_id_.end() != map_by_id_.find(id)) {
     return map_by_id_[id].get();
   }
@@ -383,6 +389,7 @@ TensorsInfo* TensorsInfoManager::GetTensorsInfo(int id) {
 TensorsInfo* TensorsInfoManager::GetTensorsInfo(ml_tensors_info_h handle) {
   ScopeLogger();
 
+  std::lock_guard<std::mutex> lock{maps_and_next_id_mutex_};
   if (map_by_handle_.end() != map_by_handle_.find(handle)) {
     return map_by_handle_[handle].get();
   }
@@ -413,6 +420,7 @@ PlatformResult TensorsInfoManager::DisposeTensorsInfo(TensorsInfo* t) {
     return PlatformResult(ErrorCode::ABORT_ERR);
   }
 
+  std::lock_guard<std::mutex> lock{maps_and_next_id_mutex_};
   map_by_handle_.erase(t->Handle());
   map_by_id_.erase(t->Id());
 

diff --git a/src/ml/ml_tensors_info_manager.h b/src/ml/ml_tensors_info_manager.h
index 96531f9fdc743aa965aff437ba9c0ab5c06e91ad..2a36d05aeccadd19d54eed61f8e926a8630c6ac4 100644 (file)
--- a/src/ml/ml_tensors_info_manager.h
+++ b/src/ml/ml_tensors_info_manager.h
@@ -18,6 +18,7 @@
 #define __ML_TENSORS_INFO_MANAGER_H__
 
 #include <map>
+#include <mutex>
 #include <vector>
 
 #include "common/logger.h"
@@ -89,6 +90,13 @@ class TensorsInfoManager {
  private:
   TensorsInfoManager(TensorsInfoManager const&) = delete;
   TensorsInfoManager& operator=(TensorsInfoManager const&) = delete;
+
+  /*
+   * For performance reasons and simplicity we use a single mutex
+   * to lock both maps and nextId_. They are often used together
+   * and we'd have to lock all of them anyway.
+   */
+  std::mutex maps_and_next_id_mutex_;
   std::map<int, std::shared_ptr<TensorsInfo>> map_by_id_;
   std::map<ml_tensors_info_h, std::shared_ptr<TensorsInfo>> map_by_handle_;
   int nextId_;