[2205] Code review #1

irods · Jul 10, 2024 · f831fbc · f831fbc
1 parent 85f89ae
commit f831fbc
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 47 deletions.
diff --git a/s3_resource/include/irods/private/s3_resource/multipart_shared_data.hpp b/s3_resource/include/irods/private/s3_resource/multipart_shared_data.hpp
@@ -1,5 +1,5 @@
-#ifndef S3_RESOURCE__MULTIPART_SHARED_DATA_HPP
-#define S3_RESOURCE__MULTIPART_SHARED_DATA_HPP
+#ifndef IRODS_S3_RESOURCE_MULTIPART_SHARED_DATA_HPP
+#define IRODS_S3_RESOURCE_MULTIPART_SHARED_DATA_HPP
 
 #include <boost/interprocess/managed_shared_memory.hpp>
 #include <boost/interprocess/containers/map.hpp>
@@ -32,13 +32,6 @@ namespace irods_s3
             , ref_count{0}
         {}
 
-        void reset_fields()
-        {
-            threads_remaining_to_close = 0;
-            number_of_threads = 0;
-            ref_count = 1;   // current object has reference so ref_count = 1
-        }
-
         bool can_delete() {
             return threads_remaining_to_close == 0;
         }
@@ -47,7 +40,6 @@ namespace irods_s3
         int number_of_threads;
         int ref_count;
     };
-
 }
 
-#endif // S3_RESOURCE__MULTIPART_SHARED_DATA_HPP
+#endif // IRODS_S3_RESOURCE_MULTIPART_SHARED_DATA_HPP
diff --git a/s3_resource/src/s3_operations.cpp b/s3_resource/src/s3_operations.cpp
@@ -70,8 +70,8 @@ using s3_transport_config = irods::experimental::io::s3_transport::config;
 
 namespace irods_s3 {
 
-    inline static const std::string  SHARED_MEMORY_KEY_PREFIX{"irods_s3-shm-"};
-    inline static const int          DEFAULT_SHARED_MEMORY_TIMEOUT_IN_SECONDS{180};
+    inline static const std::string SHARED_MEMORY_KEY_PREFIX{"irods_s3-shm-"};
+    inline static constexpr int     DEFAULT_SHARED_MEMORY_TIMEOUT_IN_SECONDS{180};
 
     // See https://groups.google.com/g/boost-list/c/5ADnEPYg-ho for an explanation
     // of why the 100*sizeof(void*) is used below.  Essentially, the shared memory
@@ -171,6 +171,11 @@ namespace irods_s3 {
         return false;
     } // end operation_requires_that_object_exists
 
+    std::string get_shmem_key(irods::plugin_context& ctx, irods::file_object_ptr file_obj) {
+        return SHARED_MEMORY_KEY_PREFIX +
+            std::to_string(std::hash<std::string>{}(get_resource_name(ctx.prop_map()) + file_obj->logical_path()));
+    }
+
     irods::error s3_file_stat_operation_with_flag_for_retry_on_not_found(irods::plugin_context& _ctx,
             struct stat* _statbuf, bool retry_on_not_found );
 
@@ -190,8 +195,7 @@ namespace irods_s3 {
         irods::file_object_ptr file_obj = boost::dynamic_pointer_cast<irods::file_object>(_ctx.fco());
 
         // Open shared memory and see if we know the number of threads from another thread
-        std::string shmem_key = SHARED_MEMORY_KEY_PREFIX +
-            std::to_string(std::hash<std::string>{}(get_resource_name(_ctx.prop_map()) + file_obj->logical_path()));
+        std::string shmem_key = get_shmem_key(_ctx, file_obj);
         logger::trace("{}:{} ({}) [[{}]] shmem_key={} hashed_string={}", __FILE__, __LINE__, __FUNCTION__, thread_id, shmem_key, get_resource_name(_ctx.prop_map()) + file_obj->logical_path() );
 
         named_shared_memory_object shm_obj{shmem_key,
@@ -1013,8 +1017,7 @@ namespace irods_s3 {
 
             // Open shared memory and get the number_of_threads 
             irods::file_object_ptr file_obj = boost::dynamic_pointer_cast<irods::file_object>(_ctx.fco());
-            std::string shmem_key = SHARED_MEMORY_KEY_PREFIX +
-                std::to_string(std::hash<std::string>{}(get_resource_name(_ctx.prop_map()) + file_obj->logical_path()));
+            std::string shmem_key = get_shmem_key(_ctx, file_obj);
             logger::trace("{}:{} ({}) [[{}]] shmem_key={} hashed_string={}", __FILE__, __LINE__, __FUNCTION__, thread_id, shmem_key, get_resource_name(_ctx.prop_map()) + file_obj->logical_path() );
 
             named_shared_memory_object shm_obj{shmem_key,
@@ -1125,19 +1128,17 @@ namespace irods_s3 {
             // Decrement the remaining to close counter in shared memory.
             // Not necessary for GET_OPR as the shared memory is not created in that instance.
             if (irods_s3::oprType != GET_OPR) {
-                std::string shmem_key = SHARED_MEMORY_KEY_PREFIX +
-                    std::to_string(std::hash<std::string>{}(get_resource_name(_ctx.prop_map()) + file_obj->logical_path()));
 
+                std::string shmem_key = get_shmem_key(_ctx, file_obj);
                 named_shared_memory_object shm_obj{shmem_key,
                     DEFAULT_SHARED_MEMORY_TIMEOUT_IN_SECONDS,
                     SHMEM_SIZE};
 
-                int open_count, ref_count;
-                std::tie(open_count, ref_count) = shm_obj.atomic_exec([](auto& data) {
+                auto [open_count, ref_count] = shm_obj.atomic_exec([](auto& data) {
                     // shmem freed when threads_remaining_to_close is zero
                     return std::make_pair(--(data.threads_remaining_to_close), data.ref_count);
                 });
-                logger::trace("{}:{} ({}) [[{}]] shmem_key={} hashed_string={} open_count={} ref_coun={}", __FILE__, __LINE__, __FUNCTION__, thread_id, shmem_key, get_resource_name(_ctx.prop_map()) + file_obj->logical_path(), open_count, ref_count);
+                logger::trace("{}:{} ({}) [[{}]] shmem_key={} hashed_string={} open_count={} ref_coun={}", __FILE__, __LINE__, __func__, thread_id, shmem_key, get_resource_name(_ctx.prop_map()) + file_obj->logical_path(), open_count, ref_count);
             }
 
             //  because s3 might not provide immediate consistency for subsequent stats,
@@ -2303,8 +2304,7 @@ namespace irods_s3 {
                 int number_of_threads = boost::lexical_cast<int>(num_threads_str);
 
                 // save the number of threads
-                std::string shmem_key = SHARED_MEMORY_KEY_PREFIX +
-                    std::to_string(std::hash<std::string>{}(get_resource_name(_ctx.prop_map()) + file_obj->logical_path()));
+                std::string shmem_key = get_shmem_key(_ctx, file_obj);
                 logger::trace("{}:{} ({}) [[{}]] shmem_key={} hashed_string={}", __FILE__, __LINE__, __FUNCTION__, thread_id, shmem_key, get_resource_name(_ctx.prop_map()) + file_obj->logical_path() );
 
                 named_shared_memory_object shm_obj{shmem_key,

diff --git a/s3_transport/include/irods/private/s3_transport/managed_shared_memory_object.hpp b/s3_transport/include/irods/private/s3_transport/managed_shared_memory_object.hpp
@@ -40,7 +40,7 @@ namespace irods::experimental::interprocess
                     , last_access_time_in_seconds(access_time)
                 {}
 
-                // T must have reset_fields() and ref_count and can_delete()
+                // T must have ref_count and can_delete()
                 T thing;
 
                 time_t last_access_time_in_seconds;
@@ -83,7 +83,7 @@ namespace irods::experimental::interprocess
 
                 if (shmem_has_expired) {
 
-                    logger::debug("{}:{} ({}) SHMEM_HAS_EXPIRED", __FILE__, __LINE__, __FUNCTION__);
+                    logger::debug("{}:{} ({}) SHMEM_HAS_EXPIRED", __FILE__, __LINE__, __func__);
 
                     // rebuild shmem object
                     shm_.destroy<ipc_object>(SHARED_DATA_NAME.c_str());
@@ -111,10 +111,10 @@ namespace irods::experimental::interprocess
                         object_->thing.~T();
                         object_ = nullptr;
                         if (!bi::shared_memory_object::remove(shm_name_.c_str())) {
-                            logger::error("{}:{} ({}) removal of shared memory object [{}] failed", __FILE__, __LINE__, __FUNCTION__, shm_name_);
+                            logger::error("{}:{} ({}) removal of shared memory object [{}] failed", __FILE__, __LINE__, __func__, shm_name_);
                         }
                         if (!bi::named_mutex::remove(shm_name_.c_str())) {
-                            logger::error("{}:{} ({}) removal of mutex for shared memory object [{}] failed", __FILE__, __LINE__, __FUNCTION__, shm_name_);
+                            logger::error("{}:{} ({}) removal of mutex for shared memory object [{}] failed", __FILE__, __LINE__, __func__, shm_name_);
                         }
                     }
                 }

diff --git a/s3_transport/include/irods/private/s3_transport/multipart_shared_data.hpp b/s3_transport/include/irods/private/s3_transport/multipart_shared_data.hpp
@@ -1,5 +1,5 @@
-#ifndef S3_TRANSPORT_MULTIPART_SHARED_DATA_HPP
-#define S3_TRANSPORT_MULTIPART_SHARED_DATA_HPP
+#ifndef IRODS_S3_TRANSPORT_MULTIPART_SHARED_DATA_HPP
+#define IRODS_S3_TRANSPORT_MULTIPART_SHARED_DATA_HPP
 
 #include <boost/interprocess/managed_shared_memory.hpp>
 #include <boost/interprocess/containers/map.hpp>
@@ -55,21 +55,6 @@ namespace irods::experimental::io::s3_transport::shared_data
             , know_number_of_threads{true}
         {}
 
-        void reset_fields()
-        {
-            threads_remaining_to_close = 0;
-            done_initiate_multipart = false;
-            upload_id = "";
-            etags.clear();
-            last_error_code = error_codes::SUCCESS;
-            cache_file_download_progress = cache_file_download_status::NOT_STARTED;
-            ref_count = 1;   // current object has reference so ref_count = 1
-            circular_buffer_read_timeout = false;
-            file_open_counter = 0;
-            cache_file_flushed = false;
-            know_number_of_threads = true;
-        }
-
         bool can_delete() {
             return know_number_of_threads
                    ? threads_remaining_to_close == 0
@@ -94,4 +79,4 @@ namespace irods::experimental::io::s3_transport::shared_data
 
 
 
-#endif // S3_TRANSPORT_MULTIPART_SHARED_DATA_HPP
+#endif // IRODS_S3_TRANSPORT_MULTIPART_SHARED_DATA_HPP
diff --git a/s3_transport/include/irods/private/s3_transport/util.hpp b/s3_transport/include/irods/private/s3_transport/util.hpp
@@ -58,7 +58,7 @@ namespace irods::experimental::io::s3_transport
         // must have enough space for the memory algorithm and reserved area but there is
         // no way of knowing the size for these.  It is stated that 100*sizeof(void*) would
         // be enough.
-        static const std::int64_t            MAX_S3_SHMEM_SIZE{100*sizeof(void*) + sizeof(shared_data::multipart_shared_data) +
+        static constexpr std::int64_t        MAX_S3_SHMEM_SIZE{100*sizeof(void*) + sizeof(shared_data::multipart_shared_data) +
                                                           MAXIMUM_NUMBER_ETAGS_PER_UPLOAD * (BYTES_PER_ETAG + 1) +
                                                           UPLOAD_ID_SIZE + 1};