maint: use synchronized_value where we use a mutex to protect data (#3992)

Author: Klaim

libmamba/include/mamba/core/execution.hpp

@@ -14,6 +14,7 @@
 #include <vector>
 
 #include "mamba/core/error_handling.hpp"
+#include "mamba/util/synchronized_value.hpp"
 
 namespace mamba
 {
@@ -71,10 +72,10 @@ namespace mamba
                 return;
             }
 
-            std::scoped_lock lock{ threads_mutex };
+            auto synched_threads = threads.synchronize();
             if (is_open)  // Double check necessary for correctness
             {
-                threads.emplace_back(std::forward<Task>(task), std::forward<Args>(args)...);
+                synched_threads->emplace_back(std::forward<Task>(task), std::forward<Args>(args)...);
             }
         }
 
@@ -92,10 +93,10 @@ namespace mamba
                 return;
             }
 
-            std::scoped_lock lock{ threads_mutex };
+            auto synched_threads = threads.synchronize();
             if (is_open)  // Double check necessary for correctness
             {
-                threads.push_back(std::move(thread));
+                synched_threads->push_back(std::move(thread));
             }
         }
 
@@ -116,12 +117,12 @@ namespace mamba
 
             invoke_close_handlers();
 
-            std::scoped_lock lock{ threads_mutex };
-            for (auto&& t : threads)
+            auto synched_threads = threads.synchronize();
+            for (auto&& t : *synched_threads)
             {
                 t.join();
             }
-            threads.clear();
+            synched_threads->clear();
         }
 
         using on_close_handler = std::function<void()>;
@@ -133,21 +134,20 @@ namespace mamba
                 return;
             }
 
-            std::scoped_lock lock{ handlers_mutex };
+            auto handlers = close_handlers.synchronize();
             if (is_open)  // Double check needed to avoid adding new handles while closing.
             {
-                close_handlers.push_back(std::move(handler));
+                handlers->push_back(std::move(handler));
             }
         }
 
     private:
 
         std::atomic<bool> is_open{ true };
-        std::vector<std::thread> threads;
-        std::recursive_mutex threads_mutex;  // TODO: replace by synchronized_value once available
-
-        std::vector<on_close_handler> close_handlers;
-        std::recursive_mutex handlers_mutex;  // TODO: replace by synchronized_value once available
+        using Threads = std::vector<std::thread>;
+        using CloseHandlers = std::vector<on_close_handler>;
+        util::synchronized_value<Threads, std::recursive_mutex> threads;
+        util::synchronized_value<CloseHandlers, std::recursive_mutex> close_handlers;
 
         void invoke_close_handlers();
     };

libmamba/include/mamba/core/thread_utils.hpp

@@ -186,15 +186,15 @@ namespace mamba
 
     inline void counting_semaphore::lock()
     {
-        std::unique_lock<std::mutex> lock(m_access_mutex);
+        std::unique_lock lock{ m_access_mutex };
         m_cv.wait(lock, [&]() { return m_value > 0; });
         --m_value;
     }
 
     inline void counting_semaphore::unlock()
     {
         {
-            std::lock_guard<std::mutex> lock(m_access_mutex);
+            std::unique_lock lock{ m_access_mutex };
             if (++m_value <= 0)
             {
                 return;

libmamba/include/mamba/download/mirror.hpp

@@ -9,14 +9,14 @@
 
 #include <functional>
 #include <memory>
-#include <mutex>
 #include <optional>
 #include <string>
 #include <vector>
 
 #include <fmt/core.h>
 
 #include "mamba/download/request.hpp"
+#include "mamba/util/synchronized_value.hpp"
 
 namespace mamba::download
 {
@@ -66,6 +66,16 @@ namespace mamba::download
         MirrorRequest& operator=(MirrorRequest&&) = default;
     };
 
+    struct MirrorStats  // Moved out of Mirror internals because of compilers not agreeing:
+                        // https://godbolt.org/z/GcjWhrb9W
+    {
+        std::optional<std::size_t> allowed_connections = std::nullopt;
+        std::size_t max_tried_connections = 0;
+        std::size_t running_transfers = 0;
+        std::size_t successful_transfers = 0;
+        std::size_t failed_transfers = 0;
+    };
+
     // A Mirror represents a location from where an asset can be downloaded.
     // It handles the generation of required requests to get the asset, and
     // provides some statistics about its usage.
@@ -109,16 +119,13 @@ namespace mamba::download
         MirrorID m_id;
         size_t m_max_retries;
 
-        // TODO: use synchronized value
-        std::mutex m_stats_mutex;
-        std::optional<std::size_t> m_allowed_connections = std::nullopt;
-        std::size_t m_max_tried_connections = 0;
-        std::size_t m_running_transfers = 0;
-        std::size_t m_successful_transfers = 0;
-        std::size_t m_failed_transfers = 0;
+        static_assert(std::default_initializable<MirrorStats>);
+
+        util::synchronized_value<MirrorStats> m_stats;
     };
 
     std::unique_ptr<Mirror> make_mirror(std::string url);
+
 }
 
 #endif

libmamba/include/mamba/util/synchronized_value.hpp

@@ -8,9 +8,11 @@
 #define MAMBA_UTIL_SYNCHRONIZED_VALUE_HPP
 
 #include <concepts>
+#include <functional>
 #include <mutex>
 #include <shared_mutex>
 #include <tuple>
+#include <utility>
 
 namespace mamba::util
 {
@@ -35,6 +37,26 @@ namespace mamba::util
     template <class T, template <class...> class U>
     constexpr bool is_type_instance_of_v = is_type_instance_of<T, U>::value;
 
+    /// `true` if the instances of two provided types can be compared with operator==.
+    /// Notice that this concept is less restrictive than `std::equality_comparable_with`,
+    /// which requires the existence of a common reference type for T and U. This additional
+    /// restriction makes it impossible to use it in the context here (originally of sparrow), where
+    /// we want to compare objects that are logically similar while being "physically" different.
+    // Source:
+    // https://github.com/man-group/sparrow/blob/66f70418cf1b00cc294c99bbbe04b5b4d2f83c98/include/sparrow/utils/mp_utils.hpp#L604-L619
+
+    template <class T, class U>
+    concept weakly_equality_comparable_with = requires(
+        const std::remove_reference_t<T>& t,
+        const std::remove_reference_t<U>& u
+    ) {
+        { t == u } -> std::convertible_to<bool>;
+        { t != u } -> std::convertible_to<bool>;
+        { u == t } -> std::convertible_to<bool>;
+        { u != t } -> std::convertible_to<bool>;
+    };
+
+
     /////////////////////////////
 
 
@@ -256,7 +278,9 @@ namespace mamba::util
 
         /// Constructs with a provided value as initializer for the stored object.
         template <typename V>
-            requires std::assignable_from<T&, V> and (not std::same_as<this_type, std::decay_t<V>>)
+            requires(not std::same_as<T, std::decay_t<V>>)
+                    and (not std::same_as<this_type, std::decay_t<V>>)
+                    and std::assignable_from<T&, V>
         synchronized_value(V&& value) noexcept
             : m_value(std::forward<V>(value))
         {
@@ -266,6 +290,11 @@ namespace mamba::util
             // the definition here.
         }
 
+        /// Constructs with a provided value as initializer for the stored object.
+        // NOTE: this is redundant with the generic impl, but required to workaround
+        // apple-clang failing to properly constrain the generic impl.
+        synchronized_value(T value) noexcept;
+
         /// Constructs with a provided initializer list used to initialize the stored object.
         template <typename V>
             requires std::constructible_from<T, std::initializer_list<V>>
@@ -284,14 +313,17 @@ namespace mamba::util
             the call. If `SharedMutex<M> == true`, the lock is a shared-lock for the provided
            `synchronized_value`'s mutex.
         */
-        template <std::equality_comparable_with<T> U, Mutex OtherMutex>
+        template <std::default_initializable U, Mutex OtherMutex>
+            requires std::assignable_from<T&, U>
         auto operator=(const synchronized_value<U, OtherMutex>& other) -> synchronized_value&;
 
         /** Locks and assign the provided value to the stored object.
             The lock is released before the end of the call.
         */
         template <typename V>
-            requires std::assignable_from<T&, V> and (not std::same_as<this_type, std::decay_t<V>>)
+            requires(not std::same_as<T, std::decay_t<V>>)
+                    and (not std::same_as<this_type, std::decay_t<V>>)
+                    and std::assignable_from<T&, V>
         auto operator=(V&& value) noexcept -> synchronized_value&
         {
             // NOTE: when moving the definition outside the class,
@@ -303,6 +335,13 @@ namespace mamba::util
             return *this;
         }
 
+        /** Locks and assign the provided value to the stored object.
+            The lock is released before the end of the call.
+        */
+        // NOTE: this is redundant with the generic impl, but required to workaround
+        // apple-clang failing to properly constrain the generic impl.
+        auto operator=(const T& value) noexcept -> synchronized_value&;
+
         /** Locks and return the value of the current object.
             The lock is released before the end of the call.
             If `SharedMutex<M> == true`, the lock is a shared-lock.
@@ -465,12 +504,12 @@ namespace mamba::util
         /** Locks (shared if possible) and compare equality of the stored object's value with the
             provided value.
         */
-        auto operator==(const std::equality_comparable_with<T> auto& other_value) const -> bool;
+        auto operator==(const weakly_equality_comparable_with<T> auto& other_value) const -> bool;
 
         /** Locks both (shared if possible) and compare equality of the stored object's value with
            the provided value.
         */
-        template <std::equality_comparable_with<T> U, Mutex OtherMutex>
+        template <weakly_equality_comparable_with<T> U, Mutex OtherMutex>
         auto operator==(const synchronized_value<U, OtherMutex>& other_value) const -> bool;
 
         auto swap(synchronized_value& other) -> void;
@@ -508,6 +547,12 @@ namespace mamba::util
     template <std::default_initializable T, Mutex M>
     synchronized_value<T, M>::synchronized_value() noexcept(std::is_nothrow_default_constructible_v<T>) = default;
 
+    template <std::default_initializable T, Mutex M>
+    synchronized_value<T, M>::synchronized_value(T value) noexcept
+        : m_value(std::move(value))
+    {
+    }
+
     template <std::default_initializable T, Mutex M>
     synchronized_value<T, M>::synchronized_value(const synchronized_value& other)
     {
@@ -516,7 +561,8 @@ namespace mamba::util
     }
 
     template <std::default_initializable T, Mutex M>
-    template <std::equality_comparable_with<T> U, Mutex OtherMutex>
+    template <std::default_initializable U, Mutex OtherMutex>
+        requires std::assignable_from<T&, U>
     auto synchronized_value<T, M>::operator=(const synchronized_value<U, OtherMutex>& other)
         -> synchronized_value<T, M>&
     {
@@ -526,6 +572,14 @@ namespace mamba::util
         return *this;
     }
 
+    template <std::default_initializable T, Mutex M>
+    auto synchronized_value<T, M>::operator=(const T& value) noexcept -> synchronized_value&
+    {
+        auto _ = lock_as_exclusive(m_mutex);
+        m_value = value;
+        return *this;
+    }
+
     template <std::default_initializable T, Mutex M>
     template <typename V>
         requires std::constructible_from<T, std::initializer_list<V>>
@@ -584,15 +638,16 @@ namespace mamba::util
     }
 
     template <std::default_initializable T, Mutex M>
-    auto synchronized_value<T, M>::operator==(const std::equality_comparable_with<T> auto& other_value
+    auto
+    synchronized_value<T, M>::operator==(const weakly_equality_comparable_with<T> auto& other_value
     ) const -> bool
     {
         auto _ = lock_as_readonly(m_mutex);
         return m_value == other_value;
     }
 
     template <std::default_initializable T, Mutex M>
-    template <std::equality_comparable_with<T> U, Mutex OtherMutex>
+    template <weakly_equality_comparable_with<T> U, Mutex OtherMutex>
     auto
     synchronized_value<T, M>::operator==(const synchronized_value<U, OtherMutex>& other_value) const
         -> bool
@@ -623,7 +678,6 @@ namespace mamba::util
     {
         return std::make_tuple(std::forward<SynchronizedValues>(sync_values).synchronize()...);
     }
-
 }
 
 #endif

libmamba/src/core/execution.cpp

@@ -8,8 +8,8 @@ namespace mamba
 
     void MainExecutor::invoke_close_handlers()
     {
-        std::scoped_lock lock{ handlers_mutex };
-        for (auto&& handler : close_handlers)
+        auto synched_handlers = close_handlers.synchronize();
+        for (auto&& handler : *synched_handlers)
         {
             const auto result = safe_invoke(handler);
             if (!result)