Removed CurlHandlerPool and simplified it

Removed CurlHandlerPool and simplified it. And against data race in OpenSSL, temporarily changed to not use Sessin cache.
ggtakec · Nov 9, 2024 · 27dd132 · 27dd132
1 parent cc29bea
commit 27dd132
Show file tree

Hide file tree

Showing 8 changed files with 17 additions and 239 deletions.
diff --git a/src/Makefile.am b/src/Makefile.am
@@ -36,7 +36,6 @@ s3fs_SOURCES = \
     metaheader.cpp \
     mpu_util.cpp \
     curl.cpp \
-    curl_handlerpool.cpp \
     curl_multi.cpp \
     curl_util.cpp \
     s3objlist.cpp \

diff --git a/src/curl.cpp b/src/curl.cpp
@@ -38,7 +38,6 @@
 #include "curl_multi.h"
 #include "curl_util.h"
 #include "s3fs_auth.h"
-#include "curl_handlerpool.h"
 #include "s3fs_cred.h"
 #include "s3fs_util.h"
 #include "string_util.h"
@@ -99,12 +98,10 @@ constexpr char   S3fsCurl::S3FS_SSL_PRIVKEY_PASSWORD[];
 std::mutex       S3fsCurl::curl_handles_lock;
 S3fsCurl::callback_locks_t S3fsCurl::callback_locks;
 bool             S3fsCurl::is_initglobal_done  = false;
-std::unique_ptr<CurlHandlerPool> S3fsCurl::sCurlPool;
-int              S3fsCurl::sCurlPoolSize       = 32;
 CURLSH*          S3fsCurl::hCurlShare          = nullptr;
 bool             S3fsCurl::is_cert_check       = true; // default
 bool             S3fsCurl::is_dns_cache        = true; // default
-bool             S3fsCurl::is_ssl_session_cache= true; // default
+bool             S3fsCurl::is_ssl_session_cache= false;// default(This turns OFF now, but turns ON again last PR)
 long             S3fsCurl::connect_timeout     = 300;  // default
 time_t           S3fsCurl::readwrite_timeout   = 120;  // default
 int              S3fsCurl::retries             = 5;    // default
@@ -162,14 +159,6 @@ bool S3fsCurl::InitS3fsCurl()
     if(!S3fsCurl::InitCryptMutex()){
         return false;
     }
-    // [NOTE]
-    // sCurlPoolSize must be over parallel(or multireq) count.
-    //
-    sCurlPoolSize = std::max({sCurlPoolSize, GetMaxParallelCount(), GetMaxMultiRequest()});
-    sCurlPool.reset(new CurlHandlerPool(sCurlPoolSize));
-    if (!sCurlPool->Init()) {
-        return false;
-    }
     return true;
 }
 
@@ -180,10 +169,6 @@ bool S3fsCurl::DestroyS3fsCurl()
     if(!S3fsCurl::DestroyCryptMutex()){
         result = false;
     }
-    if(!sCurlPool->Destroy()){
-        result = false;
-    }
-    sCurlPool.reset();
     if(!S3fsCurl::DestroyShareCurl()){
         result = false;
     }
@@ -1974,7 +1959,7 @@ bool S3fsCurl::ResetHandle()
 {
     bool run_once = curl_warnings_once.exchange(true);
 
-    sCurlPool->ResetHandler(hCurl.get());
+    curl_easy_reset(hCurl.get());
 
     if(CURLE_OK != curl_easy_setopt(hCurl, CURLOPT_NOSIGNAL, 1)){
         return false;
@@ -2097,43 +2082,37 @@ bool S3fsCurl::ResetHandle()
     return true;
 }
 
-bool S3fsCurl::CreateCurlHandle(bool only_pool, bool remake)
+bool S3fsCurl::CreateCurlHandle(bool remake)
 {
     const std::lock_guard<std::mutex> lock(S3fsCurl::curl_handles_lock);
 
     if(hCurl && remake){
-        if(!DestroyCurlHandleHasLock(false, true)){
+        if(!DestroyCurlHandleHasLock(true)){
             S3FS_PRN_ERR("could not destroy handle.");
             return false;
         }
         S3FS_PRN_INFO3("already has handle, so destroyed it or restored it to pool.");
     }
 
     if(!hCurl){
-        if(nullptr == (hCurl = sCurlPool->GetHandler(only_pool))){
-            if(!only_pool){
-                S3FS_PRN_ERR("Failed to create handle.");
-                return false;
-            }else{
-                // [NOTE]
-                // Further initialization processing is left to lazy processing to be executed later.
-                // (Currently we do not use only_pool=true, but this code is remained for the future)
-                return true;
-            }
+        hCurl.reset(curl_easy_init());
+        if(!hCurl){
+            S3FS_PRN_ERR("Failed to create handle.");
+            return false;
         }
     }
     ResetHandle();
 
     return true;
 }
 
-bool S3fsCurl::DestroyCurlHandle(bool restore_pool, bool clear_internal_data)
+bool S3fsCurl::DestroyCurlHandle(bool clear_internal_data)
 {
     const std::lock_guard<std::mutex> lock(S3fsCurl::curl_handles_lock);
-    return DestroyCurlHandleHasLock(restore_pool, clear_internal_data);
+    return DestroyCurlHandleHasLock(clear_internal_data);
 }
 
-bool S3fsCurl::DestroyCurlHandleHasLock(bool restore_pool, bool clear_internal_data)
+bool S3fsCurl::DestroyCurlHandleHasLock(bool clear_internal_data)
 {
     // [NOTE]
     // If type is REQTYPE::IAMCRED or REQTYPE::IAMROLE, do not clear type.
@@ -2150,7 +2129,7 @@ bool S3fsCurl::DestroyCurlHandleHasLock(bool restore_pool, bool clear_internal_d
 
     if(hCurl){
         S3fsCurl::curl_progress.erase(hCurl.get());
-        sCurlPool->ReturnHandler(std::move(hCurl), restore_pool);
+        hCurl.reset();
     }else{
         return false;
     }

diff --git a/src/curl.h b/src/curl.h
@@ -84,7 +84,6 @@ typedef std::unique_ptr<CURL, decltype(&curl_easy_cleanup)> CurlUniquePtr;
 //----------------------------------------------
 // class S3fsCurl
 //----------------------------------------------
-class CurlHandlerPool;
 class S3fsCred;
 class S3fsCurl;
 
@@ -131,8 +130,6 @@ class S3fsCurl
             std::mutex      ssl_session;
         } callback_locks;
         static bool             is_initglobal_done;
-        static std::unique_ptr<CurlHandlerPool> sCurlPool;
-        static int              sCurlPoolSize;
         static CURLSH*          hCurlShare;
         static bool             is_cert_check;
         static bool             is_dns_cache;
@@ -363,9 +360,9 @@ class S3fsCurl
         static bool SetIPResolveType(const char* value);
 
         // methods
-        bool CreateCurlHandle(bool only_pool = false, bool remake = false);
-        bool DestroyCurlHandle(bool restore_pool = true, bool clear_internal_data = true);
-        bool DestroyCurlHandleHasLock(bool restore_pool = true, bool clear_internal_data = true) REQUIRES(S3fsCurl::curl_handles_lock);
+        bool CreateCurlHandle(bool remake = false);
+        bool DestroyCurlHandle(bool clear_internal_data = true);
+        bool DestroyCurlHandleHasLock(bool clear_internal_data = true) REQUIRES(S3fsCurl::curl_handles_lock);
 
         bool GetIAMCredentials(const char* cred_url, const char* iam_v2_token, const char* ibm_secret_access_key, std::string& response);
         bool GetIAMRoleFromMetaData(const char* cred_url, const char* iam_v2_token, std::string& token);

diff --git a/src/curl_handlerpool.cpp b/src/curl_handlerpool.cpp
diff --git a/src/curl_handlerpool.h b/src/curl_handlerpool.h
diff --git a/src/curl_multi.cpp b/src/curl_multi.cpp
@@ -366,7 +366,7 @@ void S3fsMultiCurl::RequestPerformWrapper(S3fsCurl* s3fscurl, std::promise<int>
 
     if(!result){
         result = s3fscurl->RequestPerform();
-        s3fscurl->DestroyCurlHandle(true, false);
+        s3fscurl->DestroyCurlHandle(false);
     }
 
     const std::lock_guard<std::mutex> lock(*s3fscurl->completed_tids_lock);

diff --git a/src/fdcache_fdinfo.cpp b/src/fdcache_fdinfo.cpp
@@ -38,12 +38,6 @@
 #include "string_util.h"
 #include "threadpoolman.h"
 
-//------------------------------------------------
-// PseudoFdInfo class variables
-//------------------------------------------------
-int PseudoFdInfo::max_threads     = -1;
-int PseudoFdInfo::opt_max_threads = -1;
-
 //------------------------------------------------
 // PseudoFdInfo class methods
 //------------------------------------------------
@@ -95,7 +89,7 @@ void* PseudoFdInfo::MultipartUploadThreadWorker(void* arg)
     }else{
         S3FS_PRN_ERR("failed uploading with error(%d) [path=%s][start=%lld][size=%lld][part=%d]", result, pthparam->path.c_str(), static_cast<long long>(pthparam->start), static_cast<long long>(pthparam->size), pthparam->part_num);
     }
-    s3fscurl->DestroyCurlHandle(true, false);
+    s3fscurl->DestroyCurlHandle(false);
 
     // set result
     const std::lock_guard<std::mutex> lock(pthparam->ppseudofdinfo->upload_list_lock);

diff --git a/src/fdcache_fdinfo.h b/src/fdcache_fdinfo.h
@@ -57,9 +57,6 @@ struct pseudofdinfo_thparam
 class PseudoFdInfo
 {
     private:
-        static int              max_threads;
-        static int              opt_max_threads;    // for option value
-
         int                     pseudo_fd;
         int                     physical_fd;
         int                     flags;              // flags at open