Merge pull request #487 from evoskuil/master

Fix deadlocks resulting from iterator memory object retention.

include/bitcoin/database/impl/memory/accessor.ipp

@@ -45,7 +45,7 @@ inline void CLASS::assign(uint8_t* begin, uint8_t* end) NOEXCEPT
 }
 
 TEMPLATE
-inline uint8_t* CLASS::offset(size_t bytes) NOEXCEPT
+inline uint8_t* CLASS::offset(size_t bytes) const NOEXCEPT
 {
     if (system::is_greater(bytes, size()))
         return nullptr;

include/bitcoin/database/impl/primitives/hashmap.ipp

@@ -19,6 +19,7 @@
 #ifndef LIBBITCOIN_DATABASE_PRIMITIVES_HASHMAP_IPP
 #define LIBBITCOIN_DATABASE_PRIMITIVES_HASHMAP_IPP
 
+#include <utility>
 #include <bitcoin/system.hpp>
 #include <bitcoin/database/define.hpp>
 
@@ -148,8 +149,10 @@ Link CLASS::first(const Key& key) const NOEXCEPT
 TEMPLATE
 typename CLASS::iterator CLASS::it(const Key& key) const NOEXCEPT
 {
-    // TODO: due to iterator design, key is copied into iterator.
-    return { manager_.get(), head_.top(key), key };
+    // TODO: key is copied into iterator, ref may be better for big keys.
+    // manager.get() is called for each memory access, avoiding deadlock
+    // risk that would arise if the memory accessor was held.
+    return { manager_, head_.top(key), key };
 }
 
 TEMPLATE

include/bitcoin/database/impl/primitives/iterator.ipp

@@ -19,34 +19,28 @@
 #ifndef LIBBITCOIN_DATABASE_PRIMITIVES_ELEMENT_IPP
 #define LIBBITCOIN_DATABASE_PRIMITIVES_ELEMENT_IPP
 
-////#include <algorithm>
-////#include <utility>
+#include <cstring>
 #include <bitcoin/system.hpp>
 #include <bitcoin/database/define.hpp>
+#include <bitcoin/database/memory/memory.hpp>
 
 namespace libbitcoin {
 namespace database {
 
 TEMPLATE
-INLINE CLASS::iterator(const memory_ptr& data, const Link& start,
+INLINE CLASS::iterator(const manager& memory, const Link& start,
     const Key& key) NOEXCEPT
-  : memory_(data), key_(key), link_(start)
+  : manager_(memory), key_(key), link_(start)
 {
-    if (!is_match())
-        advance();
+    const auto ptr = get_ptr();
+    if (!is_match(ptr))
+        advance(ptr);
 }
 
 TEMPLATE
 INLINE bool CLASS::advance() NOEXCEPT
 {
-    while (!link_.is_terminal())
-    {
-        link_ = get_next();
-        if (is_match())
-            return true;
-    }
-
-    return false;
+    return advance(get_ptr());
 }
 
 TEMPLATE
@@ -59,38 +53,53 @@ INLINE const Link& CLASS::self() const NOEXCEPT
 // ----------------------------------------------------------------------------
 
 TEMPLATE
-INLINE bool CLASS::is_match() const NOEXCEPT
+INLINE memory_ptr CLASS::get_ptr() const NOEXCEPT
 {
-    using namespace system;
-    BC_ASSERT(!is_add_overflow(link_to_position(link_), Link::size));
+    return manager_.get();
+}
 
-    if (!memory_)
+TEMPLATE
+INLINE bool CLASS::advance(const memory_ptr& ptr) NOEXCEPT
+{
+    if (link_.is_terminal() || !ptr)
         return false;
 
-    auto link = memory_->offset(link_to_position(link_) + Link::size);
-    if (is_null(link))
+    do
+    {
+        link_ = get_next(ptr);
+        if (is_match(ptr))
+            return true;
+    }
+    while (!link_.is_terminal());
+    return false;
+}
+
+TEMPLATE
+INLINE bool CLASS::is_match(const memory_ptr& ptr) const NOEXCEPT
+{
+    if (link_.is_terminal() || !ptr)
         return false;
 
-    // TODO: loop unroll.
-    for (const auto& byte: key_)
-        if (byte != *(link++))
-            return false;
+    BC_ASSERT(!system::is_add_overflow(link_to_position(link_), Link::size));
+    const auto position = ptr->offset(link_to_position(link_) + Link::size);
+    if (is_null(position))
+        return false;
 
-    return true;
-    ////return std::equal(key_.begin(), key_.end(), link);
+    return is_zero(std::memcmp(key_.data(), position, key_.size()));
+    ////return std::equal(key_.begin(), key_.end(), position);
 }
 
 TEMPLATE
-INLINE Link CLASS::get_next() const NOEXCEPT
+INLINE Link CLASS::get_next(const memory_ptr& ptr) const NOEXCEPT
 {
-    if (link_.is_terminal() || !memory_)
+    if (link_.is_terminal() || !ptr)
         return Link::terminal;
 
-    const auto link = memory_->offset(link_to_position(link_));
-    if (is_null(link))
+    const auto position = ptr->offset(link_to_position(link_));
+    if (is_null(position))
         return Link::terminal;
 
-    return { system::unsafe_array_cast<uint8_t, Link::size>(link) };
+    return { system::unsafe_array_cast<uint8_t, Link::size>(position) };
 }
 
 // private
@@ -99,8 +108,7 @@ INLINE Link CLASS::get_next() const NOEXCEPT
 TEMPLATE
 constexpr size_t CLASS::link_to_position(const Link& link) NOEXCEPT
 {
-    using namespace system;
-    const auto value = possible_narrow_cast<size_t>(link.value);
+    const auto value = system::possible_narrow_cast<size_t>(link.value);
 
     if constexpr (is_slab)
     {
@@ -111,7 +119,7 @@ constexpr size_t CLASS::link_to_position(const Link& link) NOEXCEPT
     {
         // Record implies link/key independent of Size.
         constexpr auto element_size = Link::size + array_count<Key> + Size;
-        BC_ASSERT(!is_multiply_overflow(value, element_size));
+        BC_ASSERT(!system::is_multiply_overflow(value, element_size));
         return value * element_size;
     }
 }

include/bitcoin/database/impl/query/archive.ipp

@@ -354,7 +354,7 @@ bool CLASS::get_value(uint64_t& out, const output_link& link) const NOEXCEPT
 }
 
 TEMPLATE
-bool CLASS::get_unassociated(association& out, header_link link) const NOEXCEPT
+bool CLASS::get_unassociated(association& out, const header_link& link) const NOEXCEPT
 {
     if (is_associated(link))
         return false;

include/bitcoin/database/memory/accessor.hpp

@@ -45,7 +45,7 @@ class accessor
     inline void assign(uint8_t* begin, uint8_t* end) NOEXCEPT;
 
     /// Return an offset from begin, nullptr if end or past end.
-    inline uint8_t* offset(size_t bytes) NOEXCEPT override;
+    inline uint8_t* offset(size_t bytes) const NOEXCEPT override;
 
     /// The logical buffer size (from begin to end).
     inline ptrdiff_t size() const NOEXCEPT override;

include/bitcoin/database/memory/interfaces/memory.hpp

@@ -36,7 +36,7 @@ class memory
     typedef std::shared_ptr<memory> ptr;
 
     /// Return an offset from begin, nullptr if end or past end.
-    virtual uint8_t* offset(size_t value) NOEXCEPT = 0;
+    virtual uint8_t* offset(size_t value) const NOEXCEPT = 0;
 
     /// The logical buffer size (from begin to end).
     virtual ptrdiff_t size() const NOEXCEPT = 0;

include/bitcoin/database/primitives/hashmap.hpp

@@ -95,7 +95,7 @@ class hashmap
     /// Return first element or terimnal.
     Link first(const Key& key) const NOEXCEPT;
 
-    /// Iterator holds shared lock on storage remap.
+    /// Iterator walks hashmap conflict stack for matches from top down.
     iterator it(const Key& key) const NOEXCEPT;
 
     /// Return the link at the top of the conflict list (for table scanning).

include/bitcoin/database/primitives/iterator.hpp

@@ -21,21 +21,23 @@
 
 #include <bitcoin/system.hpp>
 #include <bitcoin/database/define.hpp>
+#include <bitcoin/database/primitives/manager.hpp>
 #include <bitcoin/database/memory/memory.hpp>
 
 namespace libbitcoin {
 namespace database {
 
-/// This class is not thread safe.
 /// Size non-max implies record manager (ordinal record links).
 template <typename Link, typename Key, size_t Size = max_size_t>
 class iterator
 {
 public:
     DEFAULT_COPY_MOVE_DESTRUCT(iterator);
 
+    using manager = database::manager<Link, Key, Size>;
+
     /// This advances to first match (or terminal).
-    INLINE iterator(const memory_ptr& data, const Link& start,
+    INLINE iterator(const manager& memory, const Link& start,
         const Key& key) NOEXCEPT;
 
     /// Advance to and return next iterator.
@@ -45,15 +47,17 @@ class iterator
     INLINE const Link& self() const NOEXCEPT;
 
 protected:
-    INLINE bool is_match() const NOEXCEPT;
-    INLINE Link get_next() const NOEXCEPT;
+    INLINE memory_ptr get_ptr() const NOEXCEPT;
+    INLINE bool advance(const memory_ptr& ptr) NOEXCEPT;
+    INLINE bool is_match(const memory_ptr& ptr) const NOEXCEPT;
+    INLINE Link get_next(const memory_ptr& ptr) const NOEXCEPT;
 
 private:
     static constexpr auto is_slab = (Size == max_size_t);
     static constexpr size_t link_to_position(const Link& link) NOEXCEPT;
 
     // These are thread safe.
-    const memory_ptr memory_;
+    const manager& manager_;
     const Key key_;
 
     // This is not thread safe.

include/bitcoin/database/query.hpp

@@ -319,7 +319,7 @@ class query
     /// False implies fault.
     bool get_height(size_t& out, const header_link& link) const NOEXCEPT;
     bool get_value(uint64_t& out, const output_link& link) const NOEXCEPT;
-    bool get_unassociated(association& out, header_link link) const NOEXCEPT;
+    bool get_unassociated(association& out, const header_link& link) const NOEXCEPT;
 
     inputs_ptr get_inputs(const tx_link& link) const NOEXCEPT;
     outputs_ptr get_outputs(const tx_link& link) const NOEXCEPT;

test/mocks/chunk_storage.cpp

@@ -29,18 +29,18 @@ BC_PUSH_WARNING(NO_THROW_IN_NOEXCEPT)
 
 // This is a trivial working chunk_storage interface implementation.
 chunk_storage::chunk_storage() NOEXCEPT
-  : path_{}, local_{}, buffer_{ local_ }
+  : buffer_{ local_ }, path_{ "test" }
 {
 }
 
 chunk_storage::chunk_storage(system::data_chunk& reference) NOEXCEPT
-  : path_{}, local_{}, buffer_{ reference }
+  : buffer_{ reference }, path_{ "test" }
 {
 }
 
 chunk_storage::chunk_storage(const std::filesystem::path& filename,
     size_t, size_t) NOEXCEPT
-  : path_{ filename }, local_{}, buffer_{ local_ }
+  : buffer_{ local_ }, path_{ filename }
 {
 }
 
@@ -108,13 +108,12 @@ bool chunk_storage::truncate(size_t size) NOEXCEPT
 
 size_t chunk_storage::allocate(size_t chunk) NOEXCEPT
 {
+    std::unique_lock field_lock(field_mutex_);
     if (system::is_add_overflow<size_t>(buffer_.size(), chunk))
         return chunk_storage::eof;
-
     if (buffer_.size() + chunk > buffer_.max_size())
         return chunk_storage::eof;
 
-    std::unique_lock field_lock(field_mutex_);
     std::unique_lock map_lock(map_mutex_);
     const auto link = buffer_.size();
     buffer_.resize(buffer_.size() + chunk);

test/mocks/chunk_storage.hpp

@@ -54,11 +54,14 @@ class chunk_storage
     size_t get_space() const NOEXCEPT override;
 
 private:
-    system::data_chunk local_;
+    // These are protected by mutex.
+    system::data_chunk local_{};
     system::data_chunk& buffer_;
-    mutable std::shared_mutex field_mutex_;
-    mutable std::shared_mutex map_mutex_;
-    const std::filesystem::path path_{ "test" };
+
+    // These are thread safe.
+    const std::filesystem::path path_;
+    mutable std::shared_mutex field_mutex_{};
+    mutable std::shared_mutex map_mutex_{};
 };
 
 } // namespace test