Merge pull request #2067 from upshaw-alex/add-more-clientsmanager-uni…

…t-tests

Add more unit tests for ClientsManager.

bftengine/src/bftengine/ClientsManager.cpp

@@ -58,6 +58,14 @@ ClientsManager::ClientsManager(const std::set<NodeIdType>& proxyClients,
   clientIds_.insert(externalClients_.begin(), externalClients_.end());
   clientIds_.insert(internalClients_.begin(), internalClients_.end());
   ConcordAssert(clientIds_.size() >= 1);
+
+  // For the benefit of code accessing clientsInfo_, pre-fill cliensInfo_ with a blank entry for each client to reduce
+  // ambiguity between invalid client IDs and valid client IDs for which nothing stored in clientsInfo_ has been loaded
+  // so far.
+  for (const auto& client_id : clientIds_) {
+    clientsInfo_.emplace(client_id, ClientInfo());
+  }
+
   std::ostringstream oss;
   oss << "proxy clients: ";
   std::copy(proxyClients_.begin(), proxyClients_.end(), std::ostream_iterator<NodeIdType>(oss, " "));
@@ -204,8 +212,10 @@ std::unique_ptr<ClientReplyMsg> ClientsManager::allocateNewReplyMsgAndWriteToSto
     const uint32_t sizePage = ((i < numOfPages - 1) ? sizeOfReservedPage() : sizeLastPage);
     saveReservedPage(firstPageId + i, sizePage, ptrPage);
   }
-  // now save the RSI in the rsiManager
-  rsiManager_->setRsiForClient(clientId, requestSeqNum, std::string(reply + commonMsgSize, rsiLength));
+  // now save the RSI in the rsiManager, if this ClientsManager has one.
+  if (rsiManager_) {
+    rsiManager_->setRsiForClient(clientId, requestSeqNum, std::string(reply + commonMsgSize, rsiLength));
+  }
   // we cannot set the RSI metadata before saving the reply to the reserved paged, hence save it now.
   r->setReplicaSpecificInfoLength(rsiLength);
 
@@ -251,14 +261,16 @@ std::unique_ptr<ClientReplyMsg> ClientsManager::allocateReplyFromSavedOne(NodeId
     loadReservedPage(firstPageId + i, sizePage, ptrPage);
   }
 
-  // Load the RSI data from persistent storage
-  auto rsiItem = rsiManager_->getRsiForClient(clientId, requestSeqNum);
-  auto rsiSize = rsiItem.data().size();
-  if (rsiSize > 0) {
-    auto commDataLength = r->replyLength();
-    r->setReplyLength(r->replyLength() + rsiSize);
-    memcpy(r->replyBuf() + commDataLength, rsiItem.data().data(), rsiSize);
-    r->setReplicaSpecificInfoLength(rsiSize);
+  // Load the RSI data from persistent storage, if an RSI manager is in use.
+  if (rsiManager_) {
+    auto rsiItem = rsiManager_->getRsiForClient(clientId, requestSeqNum);
+    auto rsiSize = rsiItem.data().size();
+    if (rsiSize > 0) {
+      auto commDataLength = r->replyLength();
+      r->setReplyLength(r->replyLength() + rsiSize);
+      memcpy(r->replyBuf() + commDataLength, rsiItem.data().data(), rsiSize);
+      r->setReplicaSpecificInfoLength(rsiSize);
+    }
   }
   const auto& replySeqNum = r->reqSeqNum();
   if (replySeqNum != requestSeqNum) {
@@ -344,7 +356,7 @@ bool ClientsManager::canBecomePending(NodeIdType clientId, ReqId reqSeqNum) cons
     return true;
   } catch (const std::out_of_range& e) {
     LOG_DEBUG(CL_MNGR, "no info for client: " << clientId);
-    return true;
+    return false;
   }
 }
 

bftengine/src/bftengine/ClientsManager.hpp

@@ -62,14 +62,17 @@ class ClientsManager : public ResPagesClient<ClientsManager>, public IPendingReq
 
   uint32_t numberOfRequiredReservedPages() const { return clientIds_.size() * reservedPagesPerClient_; }
 
-  // Loads any available client public keys and client replies from the reserved pages. Automatically deletes the oldest
-  // reply record for a client if a reply message is found in the reserved pages for that client but the ClientsManager
-  // already has a number of reply records for that client equalling or exceeding the maximum client batch size that was
-  // configured at the time of this ClientManager's construction (or 1 if client batching was disabled). If the
+  // Loads any available client public keys and client reply records from the reserved pages. Saves any client public
+  // keys loaded from the reserved pages to the KeyExchangeManager singleton. Automatically deletes the oldest reply
+  // record for a client if a reply message is found in the reserved pages for that client but the ClientsManager
+  // already has a number of reply records for that client equalling or exceeding the maximum client batch size that
+  // was configured at the time of this ClientManager's construction (or 1 if client batching was disabled). If the
   // ClientsManager already has existing reply records matching the client ID and sequence number of a reply found in
   // the reserved pages, the existing record will be overwritten. Automatically deletes any request records for a given
   // client with sequence numbers less than or equal to the sequence number of a reply to that client found in the
-  // reserved pages. Behavior is undefined if the applicable reserved pages contain malformed data.
+  // reserved pages. As a precondition to this function, the KeyExchangeManager singleton
+  // (KeyExchangeManager::instance()) must be fully initialized and usable. Behavior is undefined if it is not, and is
+  // also undefined if the applicable reserved pages contain malformed data.
   void loadInfoFromReservedPages();
 
   // Replies
@@ -85,9 +88,10 @@ class ClientsManager : public ResPagesClient<ClientsManager>, public IPendingReq
   // First, if this ClientsManager has a number of reply records for the given clientId equalling or exceeding the
   // maximum client batch size configured at the time of this ClientManager's construction (or 1 if client batching was
   // not enabled), deletes the oldest such record. Then, a ClientReplyMsg is allocated with the given sequence number
-  // and payload, and a copy of the message is saved to the reserved pages, and this ClientManager adds a record for
-  // this reply (potentially replacing any existing record for the given sequence number). Returns the allocated
-  // ClientReplyMsg. Behavior is undefined for all of the following cases:
+  // and payload, and a copy of the message is saved to the reserved pages (overwriting any existing reply for clientId
+  // in the reserved pages), and this ClientManager adds a record for this reply (potentially replacing any existing
+  // record for the given sequence number). Returns the allocated ClientReplyMsg. Behavior is undefined for all of the
+  // following cases:
   // - clientId does not belong to a valid client.
   // - The number of reply records this ClientsManager has for the given client is above the maximum even after the
   //   oldest one is deleted.
@@ -126,8 +130,8 @@ class ClientsManager : public ResPagesClient<ClientsManager>, public IPendingReq
   // - The number of requests this ClientsManager currently has recorded for that client is not exactly equal to the
   //   maximum client batch size configured at the time of this ClientsManager's construction (or 1 if client batching
   //   was not enabled).
-  // - This ClientsManager currently has any request or reply associated with that client recorded with ID matching
-  //   reqSeqNum.
+  // - This ClientsManager does not already have any request or reply associated with that client recorded with ID
+  //   matching reqSeqNum.
   // otherwise returns false.
   bool canBecomePending(NodeIdType clientId, ReqId reqSeqNum) const;
 
@@ -147,9 +151,10 @@ class ClientsManager : public ResPagesClient<ClientsManager>, public IPendingReq
   // Removes the current request record from the given client with the greatest sequence number if both of the following
   // are true:
   // - That greatest sequence number is greater than reqSequenceNum.
+  // - There is no current request record for the client with ID clientId and sequence number reqSequenceNum.
   // - The number of requests this ClientsManager currently has recorded for the given client is exactly equal to the
-  //   maximum client batch size configured at the time of this ClientManager's construction (or 1 if client batching
-  //   was not enabled).
+  //   global system constant maxNumOfRequestsInBatch (note this is not the same quantity as the maximum configured
+  //   client batch size).
   // Does nothing otherwise. Behavior is undefined if clientId does not belong to a valid client.
   void removeRequestsOutOfBatchBounds(NodeIdType clientId, ReqId reqSequenceNum);