kep-538 pbft_failure_detector should forget comnpleted request hashes…

… some time after they become irrelevant

pbft/pbft_failure_detector.cpp

@@ -88,8 +88,7 @@ pbft_failure_detector::request_executed(const bzn::hash_t& req_hash)
     std::lock_guard<std::mutex> 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<void()> handler)
     std::lock_guard<std::mutex> 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);
+    }
 }

pbft/pbft_failure_detector.hpp

@@ -17,9 +17,12 @@
 #include <pbft/pbft_failure_detector_base.hpp>
 #include <include/boost_asio_beast.hpp>
 #include <pbft/operations/pbft_operation.hpp>
+#include <queue>
+#include <gtest/gtest_prod.h>
 
 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<pbft_failure_detector>, public bzn::pbft_failure_detector_base
     {
@@ -32,18 +35,24 @@ namespace bzn
 
         void register_failure_handler(std::function<void()> 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<bzn::asio::io_context_base> io_context;
 
         std::unique_ptr<bzn::asio::steady_timer_base> request_progress_timer;
 
         std::list<bzn::hash_t> ordered_requests;
         std::unordered_set<bzn::hash_t> outstanding_requests;
         std::unordered_set<bzn::hash_t> completed_requests;
+        std::queue<bzn::hash_t> completed_request_queue;
 
         std::function<void()> failure_handler;
 

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; i<bzn::max_completed_requests_memory; ++i)
+        {
+            hash.str("");
+            hash << "hash_" << i;
+            this->failure_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());
+    }
+
 }