From 9b9ee3a07742040aa8b4eb883449b3864a88ba3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BC=A0=E5=AD=90=E6=81=92?= Date: Wed, 26 Jul 2023 15:48:50 +0800 Subject: [PATCH 1/6] dl_without_mutex_lock_handler_commit1 --- hybridse/src/udf/dynamic_lib_manager.cc | 34 ++++++++++++++++--------- hybridse/src/udf/dynamic_lib_manager.h | 7 ++--- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/hybridse/src/udf/dynamic_lib_manager.cc b/hybridse/src/udf/dynamic_lib_manager.cc index 8a4daef3e82..e998a969159 100644 --- a/hybridse/src/udf/dynamic_lib_manager.cc +++ b/hybridse/src/udf/dynamic_lib_manager.cc @@ -25,7 +25,8 @@ DynamicLibManager::~DynamicLibManager() { for (const auto& kv : handle_map_) { auto so_handle = kv.second; if (so_handle) { - dlclose(so_handle->handle); + // dlclose(so_handle->handle); + dlclose(so_handle); } } handle_map_.clear(); @@ -34,47 +35,53 @@ DynamicLibManager::~DynamicLibManager() { base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is_aggregate, const std::string& file, std::vector* funs) { CHECK_TRUE(funs != nullptr, common::kExternalUDFError, "funs is nullptr") - std::shared_ptr so_handle; + // std::shared_ptr so_handle; + void* so_handle; { std::lock_guard lock(mu_); auto iter = handle_map_.find(file); if (iter != handle_map_.end()) { so_handle = iter->second; - so_handle->ref_cnt++; + // so_handle->ref_cnt++; } } if (!so_handle) { void* handle = dlopen(file.c_str(), RTLD_LAZY); CHECK_TRUE(handle != nullptr, common::kExternalUDFError, "can not open the dynamic library: " + file + ", error: " + dlerror()) - so_handle = std::make_shared(handle); + // so_handle = std::make_shared(handle); + so_handle = handle; std::lock_guard lock(mu_); handle_map_.emplace(file, so_handle); } if (is_aggregate) { std::string init_fun_name = name + "_init"; - auto init_fun = dlsym(so_handle->handle, init_fun_name.c_str()); + // auto init_fun = dlsym(so_handle->handle, init_fun_name.c_str()); + auto init_fun = dlsym(so_handle, init_fun_name.c_str()); if (init_fun == nullptr) { RemoveHandler(file); return {common::kExternalUDFError, "can not find the init function: " + init_fun_name}; } funs->emplace_back(init_fun); std::string update_fun_name = name + "_update"; - auto update_fun = dlsym(so_handle->handle, update_fun_name.c_str()); + // auto update_fun = dlsym(so_handle->handle, update_fun_name.c_str()); + auto update_fun = dlsym(so_handle, update_fun_name.c_str()); if (update_fun == nullptr) { RemoveHandler(file); return {common::kExternalUDFError, "can not find the update function: " + update_fun_name}; } funs->emplace_back(update_fun); std::string output_fun_name = name + "_output"; - auto output_fun = dlsym(so_handle->handle, output_fun_name.c_str()); + // auto output_fun = dlsym(so_handle->handle, output_fun_name.c_str()); + auto output_fun = dlsym(so_handle, output_fun_name.c_str()); if (output_fun == nullptr) { RemoveHandler(file); return {common::kExternalUDFError, "can not find the output function: " + output_fun_name}; } funs->emplace_back(output_fun); } else { - auto fun = dlsym(so_handle->handle, name.c_str()); + // auto fun = dlsym(so_handle->handle, name.c_str()); + auto fun = dlsym(so_handle, name.c_str()); if (fun == nullptr) { RemoveHandler(file); return {common::kExternalUDFError, "can not find the function: " + name}; @@ -85,19 +92,22 @@ base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is } base::Status DynamicLibManager::RemoveHandler(const std::string& file) { - std::shared_ptr so_handle; + // std::shared_ptr so_handle; + void* so_handle; { std::lock_guard lock(mu_); if (auto iter = handle_map_.find(file); iter != handle_map_.end()) { - iter->second->ref_cnt--; - if (iter->second->ref_cnt == 0) { + // iter->second->ref_cnt--; + // if (iter->second->ref_cnt == 0) { + if (dlclose(iter->second) == 0) { so_handle = iter->second; handle_map_.erase(iter); } } } if (so_handle) { - CHECK_TRUE(dlclose(so_handle->handle) == 0, common::kExternalUDFError, "dlclose run error. file is " + file) + // CHECK_TRUE(dlclose(so_handle->handle) == 0, common::kExternalUDFError, "dlclose run error. file is " + file) + CHECK_TRUE(dlclose(so_handle) == 0, common::kExternalUDFError, "dlclose run error. file is " + file) } return {}; } diff --git a/hybridse/src/udf/dynamic_lib_manager.h b/hybridse/src/udf/dynamic_lib_manager.h index e5c8e4e1271..40a0c2fba2a 100644 --- a/hybridse/src/udf/dynamic_lib_manager.h +++ b/hybridse/src/udf/dynamic_lib_manager.h @@ -27,14 +27,14 @@ namespace hybridse { namespace udf { -struct DynamicLibHandle { +/*struct DynamicLibHandle { explicit DynamicLibHandle(void* ptr) { handle = ptr; ref_cnt = 1; } void* handle = nullptr; uint32_t ref_cnt = 0; -}; +};*/ class DynamicLibManager { public: @@ -50,7 +50,8 @@ class DynamicLibManager { private: std::mutex mu_; - std::map> handle_map_; + // std::map> handle_map_; + std::map handle_map_; }; } // namespace udf From d2a36ea7322ceaf1f929a57570c5c0faf44925a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BC=A0=E5=AD=90=E6=81=92?= Date: Wed, 26 Jul 2023 16:32:37 +0800 Subject: [PATCH 2/6] dl_without_mutex_lock_handler_commit2 --- hybridse/src/udf/dynamic_lib_manager.cc | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/hybridse/src/udf/dynamic_lib_manager.cc b/hybridse/src/udf/dynamic_lib_manager.cc index e998a969159..6d7580a1ad3 100644 --- a/hybridse/src/udf/dynamic_lib_manager.cc +++ b/hybridse/src/udf/dynamic_lib_manager.cc @@ -24,8 +24,9 @@ namespace udf { DynamicLibManager::~DynamicLibManager() { for (const auto& kv : handle_map_) { auto so_handle = kv.second; - if (so_handle) { - // dlclose(so_handle->handle); + // if (so_handle) { + // dlclose(so_handle->handle); + if(so_handle != nullptr) { dlclose(so_handle); } } @@ -45,7 +46,8 @@ base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is // so_handle->ref_cnt++; } } - if (!so_handle) { + // if (!so_handle) { + if (so_handle == nullptr) { void* handle = dlopen(file.c_str(), RTLD_LAZY); CHECK_TRUE(handle != nullptr, common::kExternalUDFError, "can not open the dynamic library: " + file + ", error: " + dlerror()) @@ -99,16 +101,16 @@ base::Status DynamicLibManager::RemoveHandler(const std::string& file) { if (auto iter = handle_map_.find(file); iter != handle_map_.end()) { // iter->second->ref_cnt--; // if (iter->second->ref_cnt == 0) { - if (dlclose(iter->second) == 0) { + if (iter->second != nullptr && dlclose(iter->second) == 0) { so_handle = iter->second; handle_map_.erase(iter); } } } - if (so_handle) { - // CHECK_TRUE(dlclose(so_handle->handle) == 0, common::kExternalUDFError, "dlclose run error. file is " + file) - CHECK_TRUE(dlclose(so_handle) == 0, common::kExternalUDFError, "dlclose run error. file is " + file) - } + // if (so_handle) { + // CHECK_TRUE(dlclose(so_handle->handle) == 0, common::kExternalUDFError, "dlclose run error. file is " + file) + // } + CHECK_TRUE(dlclose(so_handle) == 0, common::kExternalUDFError, "dlclose run error. file is " + file) return {}; } From ac2143c0b73aa766ddf07f4f481214badefba1ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BC=A0=E5=AD=90=E6=81=92?= Date: Tue, 1 Aug 2023 16:24:43 +0800 Subject: [PATCH 3/6] dl_without_mutex_lock_handler_commit3 --- hybridse/src/udf/dynamic_lib_manager.cc | 40 +++++-------------------- 1 file changed, 7 insertions(+), 33 deletions(-) diff --git a/hybridse/src/udf/dynamic_lib_manager.cc b/hybridse/src/udf/dynamic_lib_manager.cc index 6d7580a1ad3..8552b0c26fd 100644 --- a/hybridse/src/udf/dynamic_lib_manager.cc +++ b/hybridse/src/udf/dynamic_lib_manager.cc @@ -24,8 +24,6 @@ namespace udf { DynamicLibManager::~DynamicLibManager() { for (const auto& kv : handle_map_) { auto so_handle = kv.second; - // if (so_handle) { - // dlclose(so_handle->handle); if(so_handle != nullptr) { dlclose(so_handle); } @@ -36,29 +34,15 @@ DynamicLibManager::~DynamicLibManager() { base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is_aggregate, const std::string& file, std::vector* funs) { CHECK_TRUE(funs != nullptr, common::kExternalUDFError, "funs is nullptr") - // std::shared_ptr so_handle; - void* so_handle; + void* handle = dlopen(file.c_str(), RTLD_LAZY); + CHECK_TRUE(handle != nullptr, common::kExternalUDFError, + "can not open the dynamic library: " + file + ", error: " + dlerror()) { std::lock_guard lock(mu_); - auto iter = handle_map_.find(file); - if (iter != handle_map_.end()) { - so_handle = iter->second; - // so_handle->ref_cnt++; - } - } - // if (!so_handle) { - if (so_handle == nullptr) { - void* handle = dlopen(file.c_str(), RTLD_LAZY); - CHECK_TRUE(handle != nullptr, common::kExternalUDFError, - "can not open the dynamic library: " + file + ", error: " + dlerror()) - // so_handle = std::make_shared(handle); - so_handle = handle; - std::lock_guard lock(mu_); - handle_map_.emplace(file, so_handle); + handle_map_.[file] = so_handle; } if (is_aggregate) { std::string init_fun_name = name + "_init"; - // auto init_fun = dlsym(so_handle->handle, init_fun_name.c_str()); auto init_fun = dlsym(so_handle, init_fun_name.c_str()); if (init_fun == nullptr) { RemoveHandler(file); @@ -66,7 +50,6 @@ base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is } funs->emplace_back(init_fun); std::string update_fun_name = name + "_update"; - // auto update_fun = dlsym(so_handle->handle, update_fun_name.c_str()); auto update_fun = dlsym(so_handle, update_fun_name.c_str()); if (update_fun == nullptr) { RemoveHandler(file); @@ -74,7 +57,6 @@ base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is } funs->emplace_back(update_fun); std::string output_fun_name = name + "_output"; - // auto output_fun = dlsym(so_handle->handle, output_fun_name.c_str()); auto output_fun = dlsym(so_handle, output_fun_name.c_str()); if (output_fun == nullptr) { RemoveHandler(file); @@ -82,7 +64,6 @@ base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is } funs->emplace_back(output_fun); } else { - // auto fun = dlsym(so_handle->handle, name.c_str()); auto fun = dlsym(so_handle, name.c_str()); if (fun == nullptr) { RemoveHandler(file); @@ -94,23 +75,16 @@ base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is } base::Status DynamicLibManager::RemoveHandler(const std::string& file) { - // std::shared_ptr so_handle; void* so_handle; { std::lock_guard lock(mu_); if (auto iter = handle_map_.find(file); iter != handle_map_.end()) { - // iter->second->ref_cnt--; - // if (iter->second->ref_cnt == 0) { - if (iter->second != nullptr && dlclose(iter->second) == 0) { - so_handle = iter->second; - handle_map_.erase(iter); + if (iter->second != nullptr) { + CHECK_TRUE(handle != nullptr, common::kExternalUDFError, + "can not close the dynamic library: " + file + ", error: " + dlerror()) } } } - // if (so_handle) { - // CHECK_TRUE(dlclose(so_handle->handle) == 0, common::kExternalUDFError, "dlclose run error. file is " + file) - // } - CHECK_TRUE(dlclose(so_handle) == 0, common::kExternalUDFError, "dlclose run error. file is " + file) return {}; } From d84ce1d03074b811af14359e84c8e5b78eb5f2c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BC=A0=E5=AD=90=E6=81=92?= Date: Tue, 1 Aug 2023 17:22:38 +0800 Subject: [PATCH 4/6] dl_without_mutex_lock_handler_commit4 --- hybridse/src/udf/dynamic_lib_manager.cc | 23 ++++++++++------------- hybridse/src/udf/dynamic_lib_manager.h | 10 ---------- 2 files changed, 10 insertions(+), 23 deletions(-) diff --git a/hybridse/src/udf/dynamic_lib_manager.cc b/hybridse/src/udf/dynamic_lib_manager.cc index 8552b0c26fd..5d1309574da 100644 --- a/hybridse/src/udf/dynamic_lib_manager.cc +++ b/hybridse/src/udf/dynamic_lib_manager.cc @@ -39,32 +39,32 @@ base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is "can not open the dynamic library: " + file + ", error: " + dlerror()) { std::lock_guard lock(mu_); - handle_map_.[file] = so_handle; + handle_map_[file] = handle; } if (is_aggregate) { std::string init_fun_name = name + "_init"; - auto init_fun = dlsym(so_handle, init_fun_name.c_str()); + auto init_fun = dlsym(handle, init_fun_name.c_str()); if (init_fun == nullptr) { RemoveHandler(file); return {common::kExternalUDFError, "can not find the init function: " + init_fun_name}; } funs->emplace_back(init_fun); std::string update_fun_name = name + "_update"; - auto update_fun = dlsym(so_handle, update_fun_name.c_str()); + auto update_fun = dlsym(handle, update_fun_name.c_str()); if (update_fun == nullptr) { RemoveHandler(file); return {common::kExternalUDFError, "can not find the update function: " + update_fun_name}; } funs->emplace_back(update_fun); std::string output_fun_name = name + "_output"; - auto output_fun = dlsym(so_handle, output_fun_name.c_str()); + auto output_fun = dlsym(handle, output_fun_name.c_str()); if (output_fun == nullptr) { RemoveHandler(file); return {common::kExternalUDFError, "can not find the output function: " + output_fun_name}; } funs->emplace_back(output_fun); } else { - auto fun = dlsym(so_handle, name.c_str()); + auto fun = dlsym(handle, name.c_str()); if (fun == nullptr) { RemoveHandler(file); return {common::kExternalUDFError, "can not find the function: " + name}; @@ -75,14 +75,11 @@ base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is } base::Status DynamicLibManager::RemoveHandler(const std::string& file) { - void* so_handle; - { - std::lock_guard lock(mu_); - if (auto iter = handle_map_.find(file); iter != handle_map_.end()) { - if (iter->second != nullptr) { - CHECK_TRUE(handle != nullptr, common::kExternalUDFError, - "can not close the dynamic library: " + file + ", error: " + dlerror()) - } + std::lock_guard lock(mu_); + if (auto iter = handle_map_.find(file); iter != handle_map_.end()) { + if (iter->second != nullptr) { + CHECK_TRUE(dlclose(iter->second) == 0, common::kExternalUDFError, + "can not close the dynamic library: " + file + ", error: " + dlerror()) } } return {}; diff --git a/hybridse/src/udf/dynamic_lib_manager.h b/hybridse/src/udf/dynamic_lib_manager.h index 40a0c2fba2a..02dbd0a9a04 100644 --- a/hybridse/src/udf/dynamic_lib_manager.h +++ b/hybridse/src/udf/dynamic_lib_manager.h @@ -27,15 +27,6 @@ namespace hybridse { namespace udf { -/*struct DynamicLibHandle { - explicit DynamicLibHandle(void* ptr) { - handle = ptr; - ref_cnt = 1; - } - void* handle = nullptr; - uint32_t ref_cnt = 0; -};*/ - class DynamicLibManager { public: DynamicLibManager() = default; @@ -50,7 +41,6 @@ class DynamicLibManager { private: std::mutex mu_; - // std::map> handle_map_; std::map handle_map_; }; From 8d690c817d7f9247820594931422dcee29f0a2b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BC=A0=E5=AD=90=E6=81=92?= Date: Tue, 1 Aug 2023 18:54:20 +0800 Subject: [PATCH 5/6] dl_without_mutex_lock_handler_commit5 --- hybridse/src/udf/dynamic_lib_manager.cc | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/hybridse/src/udf/dynamic_lib_manager.cc b/hybridse/src/udf/dynamic_lib_manager.cc index 5d1309574da..903dd900db9 100644 --- a/hybridse/src/udf/dynamic_lib_manager.cc +++ b/hybridse/src/udf/dynamic_lib_manager.cc @@ -75,13 +75,19 @@ base::Status DynamicLibManager::ExtractFunction(const std::string& name, bool is } base::Status DynamicLibManager::RemoveHandler(const std::string& file) { - std::lock_guard lock(mu_); - if (auto iter = handle_map_.find(file); iter != handle_map_.end()) { - if (iter->second != nullptr) { - CHECK_TRUE(dlclose(iter->second) == 0, common::kExternalUDFError, - "can not close the dynamic library: " + file + ", error: " + dlerror()) + void* handle = nullptr; + { + std::lock_guard lock(mu_); + if (auto iter = handle_map_.find(file); iter != handle_map_.end()) { + if (iter->second != nullptr) { + handle = iter->second; + } } } + if (handle != nullptr) { + CHECK_TRUE(dlclose(handle) == 0, common::kExternalUDFError, + "can not close the dynamic library: " + file + ", error: " + dlerror()) + } return {}; } From b8d262bb5fe7f1877b6773b6bb3730c14ce7ddc0 Mon Sep 17 00:00:00 2001 From: dl239 Date: Wed, 2 Aug 2023 19:05:21 +0800 Subject: [PATCH 6/6] resolve cpplint --- hybridse/src/udf/dynamic_lib_manager.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hybridse/src/udf/dynamic_lib_manager.cc b/hybridse/src/udf/dynamic_lib_manager.cc index 2130a372a7e..37074e44f80 100644 --- a/hybridse/src/udf/dynamic_lib_manager.cc +++ b/hybridse/src/udf/dynamic_lib_manager.cc @@ -25,7 +25,7 @@ namespace udf { DynamicLibManager::~DynamicLibManager() { for (const auto& kv : handle_map_) { auto so_handle = kv.second; - if(so_handle != nullptr) { + if (so_handle != nullptr) { dlclose(so_handle); } }