Retain peers obtained earlier

src/DefaultPeerStorage.cc

@@ -54,7 +54,6 @@ namespace aria2 {
 namespace {
 
 const size_t MAX_PEER_LIST_SIZE = 512;
-const size_t MAX_PEER_LIST_UPDATE = 100;
 
 } // namespace
 
@@ -89,6 +88,14 @@ void DefaultPeerStorage::addUniqPeer(const std::shared_ptr<Peer>& peer)
 
 bool DefaultPeerStorage::addPeer(const std::shared_ptr<Peer>& peer)
 {
+  if (unusedPeers_.size() >= maxPeerListSize_) {
+    A2_LOG_DEBUG(fmt("Adding %s:%u is rejected, since unused peer list is full "
+                     "(%lu peers > %lu)",
+                     peer->getIPAddress().c_str(), peer->getPort(),
+                     static_cast<unsigned long>(unusedPeers_.size()),
+                     static_cast<unsigned long>(maxPeerListSize_)));
+    return false;
+  }
   if (isPeerAlreadyAdded(peer)) {
     A2_LOG_DEBUG(fmt("Adding %s:%u is rejected because it has been already"
                      " added.",
@@ -104,7 +111,7 @@ bool DefaultPeerStorage::addPeer(const std::shared_ptr<Peer>& peer)
   if (peerListSize >= maxPeerListSize_) {
     deleteUnusedPeer(peerListSize - maxPeerListSize_ + 1);
   }
-  unusedPeers_.push_front(peer);
+  unusedPeers_.push_back(peer);
   addUniqPeer(peer);
   A2_LOG_DEBUG(fmt("Now unused peer list contains %lu peers",
                    static_cast<unsigned long>(unusedPeers_.size())));
@@ -114,29 +121,36 @@ bool DefaultPeerStorage::addPeer(const std::shared_ptr<Peer>& peer)
 void DefaultPeerStorage::addPeer(
     const std::vector<std::shared_ptr<Peer>>& peers)
 {
-  size_t added = 0;
-  size_t addMax = std::min(maxPeerListSize_, MAX_PEER_LIST_UPDATE);
-  for (auto itr = std::begin(peers), eoi = std::end(peers);
-       itr != eoi && added < addMax; ++itr) {
-    auto& peer = *itr;
-    if (isPeerAlreadyAdded(peer)) {
-      A2_LOG_DEBUG(fmt("Adding %s:%u is rejected because it has been already"
-                       " added.",
-                       peer->getIPAddress().c_str(), peer->getPort()));
-      continue;
-    }
-    else if (isBadPeer(peer->getIPAddress())) {
-      A2_LOG_DEBUG(fmt("Adding %s:%u is rejected because it is marked bad.",
-                       peer->getIPAddress().c_str(), peer->getPort()));
-      continue;
+  if (unusedPeers_.size() < maxPeerListSize_) {
+    for (auto& peer : peers) {
+      if (isPeerAlreadyAdded(peer)) {
+        A2_LOG_DEBUG(fmt("Adding %s:%u is rejected because it has been already"
+                         " added.",
+                         peer->getIPAddress().c_str(), peer->getPort()));
+        continue;
+      }
+      else if (isBadPeer(peer->getIPAddress())) {
+        A2_LOG_DEBUG(fmt("Adding %s:%u is rejected because it is marked bad.",
+                         peer->getIPAddress().c_str(), peer->getPort()));
+        continue;
+      }
+      else {
+        A2_LOG_DEBUG(fmt(MSG_ADDING_PEER, peer->getIPAddress().c_str(),
+                         peer->getPort()));
+      }
+      unusedPeers_.push_back(peer);
+      addUniqPeer(peer);
     }
-    else {
+  }
+  else {
+    for (auto& peer : peers) {
       A2_LOG_DEBUG(
-          fmt(MSG_ADDING_PEER, peer->getIPAddress().c_str(), peer->getPort()));
+          fmt("Adding %s:%u is rejected, since unused peer list is full "
+              "(%lu peers > %lu)",
+              peer->getIPAddress().c_str(), peer->getPort(),
+              static_cast<unsigned long>(unusedPeers_.size()),
+              static_cast<unsigned long>(maxPeerListSize_)));
     }
-    unusedPeers_.push_front(peer);
-    addUniqPeer(peer);
-    ++added;
   }
   const size_t peerListSize = unusedPeers_.size();
   if (peerListSize > maxPeerListSize_) {
@@ -220,7 +234,10 @@ void DefaultPeerStorage::addBadPeer(const std::string& ipaddr)
 void DefaultPeerStorage::deleteUnusedPeer(size_t delSize)
 {
   for (; delSize > 0 && !unusedPeers_.empty(); --delSize) {
-    onErasingPeer(unusedPeers_.back());
+    auto& peer = unusedPeers_.back();
+    onErasingPeer(peer);
+    A2_LOG_DEBUG(fmt("Remove peer %s:%u", peer->getIPAddress().c_str(),
+                     peer->getOrigPort()));
     unusedPeers_.pop_back();
   }
 }

test/DefaultPeerStorageTest.cc

@@ -72,9 +72,9 @@ void DefaultPeerStorageTest::testDeleteUnusedPeer()
 {
   DefaultPeerStorage ps;
 
-  std::shared_ptr<Peer> peer1(new Peer("192.168.0.1", 6889));
-  std::shared_ptr<Peer> peer2(new Peer("192.168.0.2", 6889));
-  std::shared_ptr<Peer> peer3(new Peer("192.168.0.3", 6889));
+  auto peer1 = std::make_shared<Peer>("192.168.0.1", 6889);
+  auto peer2 = std::make_shared<Peer>("192.168.0.2", 6889);
+  auto peer3 = std::make_shared<Peer>("192.168.0.3", 6889);
 
   CPPUNIT_ASSERT(ps.addPeer(peer1));
   CPPUNIT_ASSERT(ps.addPeer(peer2));
@@ -83,7 +83,7 @@ void DefaultPeerStorageTest::testDeleteUnusedPeer()
   ps.deleteUnusedPeer(2);
 
   CPPUNIT_ASSERT_EQUAL((size_t)1, ps.getUnusedPeers().size());
-  CPPUNIT_ASSERT_EQUAL(std::string("192.168.0.3"),
+  CPPUNIT_ASSERT_EQUAL(std::string("192.168.0.1"),
                        ps.getUnusedPeers()[0]->getIPAddress());
 
   ps.deleteUnusedPeer(100);
@@ -97,20 +97,20 @@ void DefaultPeerStorageTest::testAddPeer()
   ps.setMaxPeerListSize(2);
   ps.setBtRuntime(btRuntime);
 
-  std::shared_ptr<Peer> peer1(new Peer("192.168.0.1", 6889));
-  std::shared_ptr<Peer> peer2(new Peer("192.168.0.2", 6889));
-  std::shared_ptr<Peer> peer3(new Peer("192.168.0.3", 6889));
+  auto peer1 = std::make_shared<Peer>("192.168.0.1", 6889);
+  auto peer2 = std::make_shared<Peer>("192.168.0.2", 6889);
+  auto peer3 = std::make_shared<Peer>("192.168.0.3", 6889);
 
   CPPUNIT_ASSERT(ps.addPeer(peer1));
   CPPUNIT_ASSERT(ps.addPeer(peer2));
-  CPPUNIT_ASSERT(ps.addPeer(peer3));
+  CPPUNIT_ASSERT(!ps.addPeer(peer3));
 
   CPPUNIT_ASSERT_EQUAL((size_t)2, ps.getUnusedPeers().size());
-  CPPUNIT_ASSERT_EQUAL(std::string("192.168.0.3"),
+  CPPUNIT_ASSERT_EQUAL(std::string("192.168.0.1"),
                        ps.getUnusedPeers()[0]->getIPAddress());
 
   CPPUNIT_ASSERT(!ps.addPeer(peer2));
-  CPPUNIT_ASSERT(ps.addPeer(peer1));
+  CPPUNIT_ASSERT(!ps.addPeer(peer3));
 
   CPPUNIT_ASSERT_EQUAL((size_t)2, ps.getUnusedPeers().size());
   CPPUNIT_ASSERT_EQUAL(std::string("192.168.0.1"),
@@ -137,20 +137,24 @@ void DefaultPeerStorageTest::testIsPeerAvailable()
 void DefaultPeerStorageTest::testCheckoutPeer()
 {
   DefaultPeerStorage ps;
-  std::shared_ptr<Peer> peers[] = {
-      std::shared_ptr<Peer>(new Peer("192.168.0.1", 1000)),
-      std::shared_ptr<Peer>(new Peer("192.168.0.2", 1000)),
-      std::shared_ptr<Peer>(new Peer("192.168.0.3", 1000))};
-  int len = arraySize(peers);
-  for (int i = 0; i < len; ++i) {
-    ps.addPeer(peers[i]);
+
+  auto peers = {
+      std::make_shared<Peer>("192.168.0.1", 1000),
+      std::make_shared<Peer>("192.168.0.2", 1000),
+      std::make_shared<Peer>("192.168.0.3", 1000),
+  };
+
+  for (auto& peer : peers) {
+    ps.addPeer(peer);
   }
-  for (int i = 0; i < len; ++i) {
-    std::shared_ptr<Peer> peer = ps.checkoutPeer(i + 1);
-    CPPUNIT_ASSERT_EQUAL(peers[len - i - 1]->getIPAddress(),
-                         peer->getIPAddress());
+
+  int i = 0;
+  for (auto& peer : peers) {
+    auto p = ps.checkoutPeer(i + 1);
+    ++i;
+    CPPUNIT_ASSERT_EQUAL(peer->getIPAddress(), p->getIPAddress());
   }
-  CPPUNIT_ASSERT(!ps.checkoutPeer(len + 1));
+  CPPUNIT_ASSERT(!ps.checkoutPeer(peers.size() + 1));
 }
 
 void DefaultPeerStorageTest::testReturnPeer()