LockFile behavior on file-locking is now almost independent from Context (#2608)

LockFile behavior on file-locking is now almost independent from Context

---------

Co-authored-by: Johan Mabille <johan.mabille@gmail.com>

libmamba/include/mamba/core/util.hpp

@@ -121,6 +121,31 @@ namespace mamba
 
     class LockFileOwner;
 
+    // @return `true` if constructing a `LockFile` will result in locking behavior, `false` if
+    // using `LockFile will not lock the file and behave like a no-op.
+    //
+    // @warning This function must be called in the execution scope `main()`, doing otherwise leads
+    // to undefined behavior.
+    //
+    // @warning This is a thread-safe accessor for a global parameter: the returned value is
+    // therefore obsolete before being obtained and should be considered as a hint.
+    bool is_file_locking_allowed();
+
+
+    // Controls if, with `true`, constructing a `LockFile` will result in locking behavior,
+    // or, with `false, will not lock the file and behave like a no-op.
+    //
+    // @warning This function must be called in the execution scope `main()`, doing otherwise leads
+    // to undefined behavior.
+    //
+    // @warning This is a thread-safe function setting a global parameter: if concurrent threads
+    // are both calling this function with different value there is no guarantee as to which
+    // value will be retained.
+    // However if there is exactly one thread executing this function then the following is true:
+    //    const auto result = allow_file_locking(allow);
+    //    result == allow && is_file_locking_allowed() == allow
+    bool allow_file_locking(bool allow);
+
     // This is a non-throwing file-locking mechanism.
     // It can be used on a file or directory path. In the case of a directory path a file will be
     // created to be locked. The locking will be implemented using the OS's filesystem locking
@@ -137,12 +162,12 @@ namespace mamba
     // Basically, all instacnes of `LockFile` locking the same path are sharing the lock, which will
     // only be released once there is no instance alive.
     //
-    // Use `Context::instance().use_lockfiles = false` to never have locking happen, in which case
+    // Use `mamba::allow_file_locking(false)` to never have locking happen, in which case
     // the created `LockFile` instance will not be locked (`is_locked()` will return false) but will
     // have no error either (`error()` will return `noopt`).
     //
     // Example:
-    //
+    //      using namespace mamba;
     //      LockFile some_work_on(some_path)
     //      {
     //          LockFile lock{ some_path, timeout };
@@ -171,8 +196,8 @@ namespace mamba
         // Non-throwing constructors, attempting lock on the provided path, file or directory.
         // In case of a directory, a lock-file will be created, located at `this->lockfile_path()`
         // and `this->is_locked()` (and `if(*this))` will always return true (unless this instance
-        // is moved-from). If the lock acquisition failed or `Context::instance().use_lockfiles ==
-        // false` and until re-assigned:
+        // is moved-from). If the lock acquisition failed or `allow_file_locking(false)` and until
+        // re-assigned:
         // - `this->is_locked() == false` and `if(*this) ...` will go in the `false` branch.
         // - accessors will throw, except `is_locked()`, `count_lock_owners()`, and `error()`
         LockFile(const fs::u8path& path);

libmamba/src/api/configuration.cpp

@@ -1841,6 +1841,8 @@ namespace mamba
         }
         m_load_lock = false;
 
+        allow_file_locking(Context::instance().use_lockfiles);
+
         LOG_DEBUG << m_config.size() << " configurables computed";
 
         if (this->at("print_config_only").value<bool>())

libmamba/src/core/run.cpp

@@ -194,7 +194,7 @@ namespace mamba
         : location{ proc_dir() / fmt::format("{}.json", getpid()) }
     {
         // Lock must be hold for the duraction of this constructor.
-        if (Context::instance().use_lockfiles)
+        if (is_file_locking_allowed())
         {
             assert(proc_dir_lock);
         }

libmamba/src/core/util.cpp

@@ -962,11 +962,19 @@ namespace mamba
             LockedFilesRegistry& operator=(LockedFilesRegistry&&) = delete;
             LockedFilesRegistry& operator=(const LockedFilesRegistry&) = delete;
 
+            bool is_file_locking_allowed() const
+            {
+                return m_is_file_locking_allowed;
+            }
+            bool allow_file_locking(bool allow)
+            {
+                return m_is_file_locking_allowed.exchange(allow);
+            }
 
             tl::expected<std::shared_ptr<LockFileOwner>, mamba_error>
             acquire_lock(const fs::u8path& file_path, const std::chrono::seconds timeout)
             {
-                if (!Context::instance().use_lockfiles)
+                if (!m_is_file_locking_allowed)
                 {
                     // No locking allowed, so do nothing.
                     return std::shared_ptr<LockFileOwner>{};
@@ -1032,6 +1040,8 @@ namespace mamba
 
         private:
 
+            std::atomic_bool m_is_file_locking_allowed{ true };
+
             // TODO: replace by something like boost::multiindex or equivalent to avoid having to
             // handle 2 hashmaps
             std::unordered_map<fs::u8path, std::weak_ptr<LockFileOwner>> locked_files;  // TODO:
@@ -1056,6 +1066,16 @@ namespace mamba
         static LockedFilesRegistry files_locked_by_this_process;
     }
 
+    bool is_file_locking_allowed()
+    {
+        return files_locked_by_this_process.is_file_locking_allowed();
+    }
+
+    bool allow_file_locking(bool allow)
+    {
+        return files_locked_by_this_process.allow_file_locking(allow);
+    }
+
     bool LockFileOwner::lock_non_blocking()
     {
         if (files_locked_by_this_process.is_locked(m_lockfile_path))

libmamba/tests/src/core/test_lockfile.cpp

@@ -56,7 +56,7 @@ namespace mamba
 
             ~LockDirTest()
             {
-                mamba::Context::instance().use_lockfiles = true;
+                mamba::allow_file_locking(true);
                 spdlog::set_level(spdlog::level::info);
             }
         };
@@ -79,14 +79,14 @@ namespace mamba
             TEST_CASE_FIXTURE(LockDirTest, "disable_locking")
             {
                 {
-                    auto _ = on_scope_exit([] { mamba::Context::instance().use_lockfiles = true; });
-                    mamba::Context::instance().use_lockfiles = false;
+                    auto _ = on_scope_exit([] { mamba::allow_file_locking(true); });
+                    mamba::allow_file_locking(false);
                     auto lock = LockFile(tempdir_path);
                     CHECK_FALSE(lock);
                 }
-                REQUIRE(mamba::Context::instance().use_lockfiles);
+                REQUIRE(mamba::is_file_locking_allowed());
                 {
-                    REQUIRE(mamba::Context::instance().use_lockfiles);
+                    REQUIRE(mamba::is_file_locking_allowed());
                     auto lock = LockFile(tempdir_path);
                     CHECK(lock);
                 }

libmambapy/libmambapy/__init__.pyi

@@ -789,7 +789,7 @@ class Context:
         :type: bool
         """
     @use_lockfiles.setter
-    def use_lockfiles(self, arg0: bool) -> None:
+    def use_lockfiles(self, arg1: bool) -> None:
         pass
     @property
     def use_only_tar_bz2(self) -> bool:

libmambapy/src/main.cpp

@@ -533,7 +533,19 @@ PYBIND11_MODULE(bindings, m)
                            " The new error messages are always enabled.");
             }
         )
-        .def_readwrite("use_lockfiles", &Context::use_lockfiles)
+        .def_property(
+            "use_lockfiles",
+            [](Context& ctx)
+            {
+                ctx.use_lockfiles = is_file_locking_allowed();
+                return ctx.use_lockfiles;
+            },
+            [](Context& ctx, bool allow)
+            {
+                allow_file_locking(allow);
+                ctx.use_lockfiles = allow;
+            }
+        )
         .def("set_verbosity", &Context::set_verbosity)
         .def("set_log_level", &Context::set_log_level);