node: fix mdbx::env lifecycle in db::ROAccess and db::RWAccess (#1772)

erigontech · Jan 24, 2024 · f34e532 · f34e532
1 parent 4780733
commit f34e532
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 10 deletions.
diff --git a/silkworm/node/db/mdbx.hpp b/silkworm/node/db/mdbx.hpp
@@ -29,6 +29,8 @@
 #include <mdbx.h++>
 #pragma GCC diagnostic pop
 
+#include <utility>
+
 #include <absl/functional/function_ref.h>
 
 #include <silkworm/core/common/base.hpp>
@@ -336,21 +338,21 @@ class RWTxnUnmanaged : public RWTxn, protected ::mdbx::txn {
 //! \brief This class create ROTxn(s) on demand, it is used to enforce in some method signatures the type of db access
 class ROAccess {
   public:
-    explicit ROAccess(mdbx::env& env) : env_{env} {}
+    explicit ROAccess(mdbx::env env) : env_{std::move(env)} {}
     ROAccess(const ROAccess& copy) = default;
 
     ROTxnManaged start_ro_tx() { return ROTxnManaged(env_); }
 
     mdbx::env& operator*() { return env_; }
 
   protected:
-    mdbx::env& env_;
+    mdbx::env env_;
 };
 
 //! \brief This class create RWTxn(s) on demand, it is used to enforce in some method signatures the type of db access
 class RWAccess : public ROAccess {
   public:
-    explicit RWAccess(mdbx::env& env) : ROAccess{env} {}
+    explicit RWAccess(mdbx::env env) : ROAccess{std::move(env)} {}
     RWAccess(const RWAccess& copy) = default;
 
     RWTxnManaged start_rw_tx() { return RWTxnManaged(env_); }
@@ -524,7 +526,7 @@ inline std::filesystem::path get_datafile_path(const std::filesystem::path& base
 }
 
 //! \brief Defines the direction of cursor while looping by cursor_for_each or cursor_for_count
-enum class CursorMoveDirection {
+enum class CursorMoveDirection : uint8_t {
     Forward,
     Reverse
 };

diff --git a/silkworm/node/db/mdbx_test.cpp b/silkworm/node/db/mdbx_test.cpp
@@ -146,8 +146,7 @@ TEST_CASE("Environment opening") {
 
 TEST_CASE("Cursor") {
     const TemporaryDirectory tmp_dir;
-    db::EnvConfig db_config{tmp_dir.path().string(), /*create*/ true};
-    db_config.in_memory = true;
+    db::EnvConfig db_config{.path = tmp_dir.path().string(), .create = true, .in_memory = true};
     auto env{db::open_env(db_config)};
 
     const db::MapConfig map_config{"GeneticCode"};
@@ -232,10 +231,36 @@ TEST_CASE("Cursor") {
     REQUIRE(other_thread_size2 == 0);
 }
 
+TEST_CASE("ROAccess/RWAccess ::mdbx::env lifecycle") {
+    const TemporaryDirectory tmp_dir;
+    db::EnvConfig db_config{.path = tmp_dir.path().string(), .create = true, .in_memory = true};
+    auto env{db::open_env(db_config)};
+
+    SECTION("ROAccess") {
+        std::unique_ptr<db::ROAccess> access;
+        {
+            // Create ROAccess with env_copy lvalue that gets destroyed immediately after
+            ::mdbx::env env_copy{env};  // NOLINT(cppcoreguidelines-slicing)
+            access = std::make_unique<db::ROAccess>(env_copy);
+        }
+        // env is still alive so using access *must* be safe
+        CHECK(access->operator*().get_path() == env.get_path());
+    }
+    SECTION("RWAccess") {
+        std::unique_ptr<db::RWAccess> access;
+        {
+            // Create RWAccess with env_copy lvalue that gets destroyed immediately after
+            ::mdbx::env env_copy{env};  // NOLINT(cppcoreguidelines-slicing)
+            access = std::make_unique<db::RWAccess>(env_copy);
+        }
+        // env is still alive so using access *must* be safe
+        CHECK(access->operator*().get_path() == env.get_path());
+    }
+}
+
 TEST_CASE("RWTxn") {
     const TemporaryDirectory tmp_dir;
-    db::EnvConfig db_config{tmp_dir.path().string(), /*create*/ true};
-    db_config.in_memory = true;
+    db::EnvConfig db_config{.path = tmp_dir.path().string(), .create = true, .in_memory = true};
     auto env{db::open_env(db_config)};
     static const char* table_name{"GeneticCode"};
 
@@ -351,8 +376,7 @@ TEST_CASE("RWTxn") {
 
 TEST_CASE("Cursor walk") {
     const TemporaryDirectory tmp_dir;
-    db::EnvConfig db_config{tmp_dir.path().string(), /*create*/ true};
-    db_config.in_memory = true;
+    db::EnvConfig db_config{.path = tmp_dir.path().string(), .create = true, .in_memory = true};
     auto env{db::open_env(db_config)};
     auto txn{env.start_write()};