From feb39a154f6301181565ef926832c2080dc5a634 Mon Sep 17 00:00:00 2001 From: rnistuk Date: Fri, 1 Mar 2019 15:22:09 -0800 Subject: [PATCH] kep-538 pbft_failure_detector should forget comnpleted request hashes some time after they become irrelevant --- pbft/pbft_failure_detector.cpp | 17 +++++++- pbft/pbft_failure_detector.hpp | 9 ++++ pbft/test/pbft_failure_detector_test.cpp | 55 +++++++++++++++++++++++- 3 files changed, 78 insertions(+), 3 deletions(-) diff --git a/pbft/pbft_failure_detector.cpp b/pbft/pbft_failure_detector.cpp index f9c4c3c4..ba4f3368 100644 --- a/pbft/pbft_failure_detector.cpp +++ b/pbft/pbft_failure_detector.cpp @@ -88,8 +88,7 @@ pbft_failure_detector::request_executed(const bzn::hash_t& req_hash) std::lock_guard lock(this->lock); this->outstanding_requests.erase(req_hash); - this->completed_requests.emplace(req_hash); - // TODO KEP-538: Need to garbage collect completed_requests eventually + this->add_completed_request_hash(req_hash); } void @@ -98,4 +97,18 @@ pbft_failure_detector::register_failure_handler(std::function handler) std::lock_guard lock(this->lock); this->failure_handler = handler; +} + +void +pbft_failure_detector::add_completed_request_hash(const bzn::hash_t& request_hash) +{ + this->completed_requests.emplace(request_hash); + this->completed_request_queue.push(request_hash); + + if (max_completed_requests_memory < this->completed_requests.size()) + { + auto old_hash = this->completed_request_queue.front(); + this->completed_request_queue.pop(); + this->completed_requests.erase(old_hash); + } } \ No newline at end of file diff --git a/pbft/pbft_failure_detector.hpp b/pbft/pbft_failure_detector.hpp index f83fbb65..90c5e90a 100644 --- a/pbft/pbft_failure_detector.hpp +++ b/pbft/pbft_failure_detector.hpp @@ -17,9 +17,12 @@ #include #include #include +#include +#include namespace bzn { + const uint32_t max_completed_requests_memory {10000}; // TODO - consider making this a configurable option class pbft_failure_detector : public std::enable_shared_from_this, public bzn::pbft_failure_detector_base { @@ -32,11 +35,16 @@ namespace bzn void register_failure_handler(std::function handler) override; + FRIEND_TEST(pbft_failure_detector_test, add_completed_request_hash_must_update_completed_requests_and_completed_request_queue); + FRIEND_TEST(pbft_failure_detector_test, add_completed_request_hash_must_garbage_collect); + private: void start_timer(); void handle_timeout(boost::system::error_code ec); + void add_completed_request_hash(const bzn::hash_t& request_hash); + std::shared_ptr io_context; std::unique_ptr request_progress_timer; @@ -44,6 +52,7 @@ namespace bzn std::list ordered_requests; std::unordered_set outstanding_requests; std::unordered_set completed_requests; + std::queue completed_request_queue; std::function failure_handler; diff --git a/pbft/test/pbft_failure_detector_test.cpp b/pbft/test/pbft_failure_detector_test.cpp index 8c5193d9..f8b293c5 100644 --- a/pbft/test/pbft_failure_detector_test.cpp +++ b/pbft/test/pbft_failure_detector_test.cpp @@ -19,7 +19,7 @@ using namespace ::testing; -namespace +namespace bzn { class pbft_failure_detector_test : public Test @@ -134,5 +134,58 @@ namespace this->request_timer_callback(boost::system::error_code()); } + TEST_F(pbft_failure_detector_test, add_completed_request_hash_must_update_completed_requests_and_completed_request_queue) + { + this->build_failure_detector(); + + EXPECT_TRUE( this->failure_detector->completed_requests.empty()); + EXPECT_TRUE( this->failure_detector->completed_request_queue.empty()); + + this->failure_detector->add_completed_request_hash("request_hash_000"); + + EXPECT_EQ( size_t(1), this->failure_detector->completed_requests.size()); + EXPECT_EQ( size_t(1), this->failure_detector->completed_request_queue.size()); + } + + TEST_F(pbft_failure_detector_test, add_completed_request_hash_must_garbage_collect) + { + this->build_failure_detector(); + + EXPECT_TRUE( this->failure_detector->completed_requests.empty()); + EXPECT_TRUE( this->failure_detector->completed_request_queue.empty()); + + // load up the completed request hash containers + std::stringstream hash; + for(size_t i=0; ifailure_detector->add_completed_request_hash(hash.str()); + } + EXPECT_EQ( size_t(bzn::max_completed_requests_memory), this->failure_detector->completed_requests.size()); + EXPECT_EQ( size_t(bzn::max_completed_requests_memory), this->failure_detector->completed_request_queue.size()); + + EXPECT_FALSE(this->failure_detector->completed_requests.end() == this->failure_detector->completed_requests.find("hash_0")); + EXPECT_EQ("hash_0", this->failure_detector->completed_request_queue.front()); + + // Garbage collection should start to take effect + this->failure_detector->add_completed_request_hash("extra_hash_000"); + + EXPECT_EQ( size_t(bzn::max_completed_requests_memory), this->failure_detector->completed_requests.size()); + EXPECT_EQ( size_t(bzn::max_completed_requests_memory), this->failure_detector->completed_request_queue.size()); + EXPECT_TRUE(this->failure_detector->completed_requests.end() == this->failure_detector->completed_requests.find("hash_0")); + EXPECT_EQ("hash_1", this->failure_detector->completed_request_queue.front()); + + this->failure_detector->add_completed_request_hash("extra_hash_001"); + this->failure_detector->add_completed_request_hash("extra_hash_002"); + this->failure_detector->add_completed_request_hash("extra_hash_003"); + this->failure_detector->add_completed_request_hash("extra_hash_004"); + + EXPECT_EQ( size_t(bzn::max_completed_requests_memory), this->failure_detector->completed_requests.size()); + EXPECT_EQ( size_t(bzn::max_completed_requests_memory), this->failure_detector->completed_request_queue.size()); + EXPECT_TRUE(this->failure_detector->completed_requests.end() == this->failure_detector->completed_requests.find("hash_4")); + EXPECT_EQ("hash_5", this->failure_detector->completed_request_queue.front()); + } + }