2008-09-19 Tatsuhiro Tsujikawa <tujikawa at rednoah dot com>

Fixed the bug that a block in a piece is requested when same 
block is
	already requested to the same peer in end game mode.
	* src/BtRequestFactory.h
	* src/DefaultBtInteractive.cc
	* src/DefaultBtRequestFactory.cc
	* src/DefaultBtRequestFactory.h
	* src/DefaultPieceStorage.cc
	* src/DefaultPieceStorage.h
	* src/PieceStorage.h
	* src/UnknownLengthPieceStorage.cc
	* src/UnknownLengthPieceStorage.h
	* test/DefaultBtRequestFactoryTest.cc
	* test/DefaultPieceStorageTest.cc
	* test/MockBtRequestFactory.h
	* test/MockPieceStorage.h

ChangeLog

@@ -1,3 +1,21 @@
+2008-09-19  Tatsuhiro Tsujikawa  <tujikawa at rednoah dot com>
+
+	Fixed the bug that a block in a piece is requested when same block is
+	already requested to the same peer in end game mode.
+	* src/BtRequestFactory.h
+	* src/DefaultBtInteractive.cc
+	* src/DefaultBtRequestFactory.cc
+	* src/DefaultBtRequestFactory.h
+	* src/DefaultPieceStorage.cc
+	* src/DefaultPieceStorage.h
+	* src/PieceStorage.h
+	* src/UnknownLengthPieceStorage.cc
+	* src/UnknownLengthPieceStorage.h
+	* test/DefaultBtRequestFactoryTest.cc
+	* test/DefaultPieceStorageTest.cc
+	* test/MockBtRequestFactory.h
+	* test/MockPieceStorage.h
+
 2008-09-19  Tatsuhiro Tsujikawa  <tujikawa at rednoah dot com>
 
 	Removed _uploadLength and _downloadLength from PeerSessionResource

src/BtRequestFactory.h

@@ -76,6 +76,13 @@ public:
    */
   virtual void createRequestMessagesOnEndGame
   (std::deque<SharedHandle<BtMessage> >& requests, size_t max) = 0;
+
+  /**
+   * Stores the list of index of pieces added using addTargetPiece() into
+   * indexes.
+   */
+  virtual void getTargetPieceIndexes(std::deque<size_t>& indexes) const = 0;
+
 };
 
 typedef SharedHandle<BtRequestFactory> BtRequestFactoryHandle;

src/DefaultBtInteractive.cc

@@ -295,25 +295,32 @@ void DefaultBtInteractive::fillPiece(size_t maxMissingBlock) {
 
     if(peer->peerChoking()) {
       if(peer->isFastExtensionEnabled()) {
-
+	std::deque<size_t> excludedIndexes;
+	btRequestFactory->getTargetPieceIndexes(excludedIndexes);
 	while(numMissingBlock < maxMissingBlock) {
-	  PieceHandle piece = pieceStorage->getMissingFastPiece(peer);
+	  SharedHandle<Piece> piece =
+	    pieceStorage->getMissingFastPiece(peer, excludedIndexes);
 	  if(piece.isNull()) {
 	    break;
 	  } else {
 	    btRequestFactory->addTargetPiece(piece);
 	    numMissingBlock += piece->countMissingBlock();
+	    excludedIndexes.push_back(piece->getIndex());
 	  }
 	}
       }
     } else {
+      std::deque<size_t> excludedIndexes;
+      btRequestFactory->getTargetPieceIndexes(excludedIndexes);
       while(numMissingBlock < maxMissingBlock) {
-	PieceHandle piece = pieceStorage->getMissingPiece(peer);
+	SharedHandle<Piece> piece =
+	  pieceStorage->getMissingPiece(peer, excludedIndexes);
 	if(piece.isNull()) {
 	  break;
 	} else {
 	  btRequestFactory->addTargetPiece(piece);
 	  numMissingBlock += piece->countMissingBlock();
+	  excludedIndexes.push_back(piece->getIndex());
 	}
       }
     }

src/DefaultBtRequestFactory.cc

@@ -210,6 +210,13 @@ size_t DefaultBtRequestFactory::countMissingBlock()
 		       CountMissingBlock()).getNumMissingBlock();
 }
 
+void DefaultBtRequestFactory::getTargetPieceIndexes
+(std::deque<size_t>& indexes) const
+{
+  std::transform(pieces.begin(), pieces.end(), std::back_inserter(indexes),
+		 mem_fun_sh(&Piece::getIndex));
+}
+
 std::deque<SharedHandle<Piece> >& DefaultBtRequestFactory::getTargetPieces()
 {
   return pieces;

src/DefaultBtRequestFactory.h

@@ -84,6 +84,8 @@ public:
   virtual void createRequestMessagesOnEndGame
   (std::deque<SharedHandle<BtMessage> >& requests, size_t max);
 
+  virtual void getTargetPieceIndexes(std::deque<size_t>& indexes) const;
+
   std::deque<SharedHandle<Piece> >& getTargetPieces();
 
   void setCuid(int32_t cuid)

src/DefaultPieceStorage.cc

@@ -87,22 +87,20 @@ bool DefaultPieceStorage::isEndGame()
   return bitfieldMan->countMissingBlock() <= endGamePieceNum;
 }
 
-bool DefaultPieceStorage::getMissingPieceIndex(size_t& index, const PeerHandle& peer)
+bool DefaultPieceStorage::getMissingPieceIndex(size_t& index,
+					       const unsigned char* bitfield,
+					       size_t& length)
 {
   std::deque<size_t> indexes;
   bool r;
   if(isEndGame()) {
-    r = bitfieldMan->getAllMissingIndexes(indexes, peer->getBitfield(),
+    r = bitfieldMan->getAllMissingIndexes(indexes, bitfield, length);
-					  peer->getBitfieldLength());
   } else {
-    r = bitfieldMan->getAllMissingUnusedIndexes(indexes,
+    r = bitfieldMan->getAllMissingUnusedIndexes(indexes, bitfield, length);
-						peer->getBitfield(),
-						peer->getBitfieldLength());
   }
   if(r) {
     // We assume indexes is sorted using comparator less.
-    _pieceSelector->select(index, indexes);
+    return _pieceSelector->select(index, indexes);
-    return true;
   } else {
     return false;
   }
@@ -171,45 +169,81 @@ PieceHandle DefaultPieceStorage::findUsedPiece(size_t index) const
   }
 }
 
-PieceHandle DefaultPieceStorage::getMissingPiece(const PeerHandle& peer)
+SharedHandle<Piece> DefaultPieceStorage::getMissingPiece
+(const unsigned char* bitfield, size_t length)
 {
   size_t index;
-  if(getMissingPieceIndex(index, peer)) {
+  if(getMissingPieceIndex(index, bitfield, length)) {
     return checkOutPiece(index);
   } else {
     return SharedHandle<Piece>();
   }
 }
 
-bool DefaultPieceStorage::getMissingFastPieceIndex(size_t& index,
+SharedHandle<Piece> DefaultPieceStorage::getMissingPiece
-						   const PeerHandle& peer)
+(const BitfieldMan& bitfield)
+{
+  return getMissingPiece(bitfield.getBitfield(), bitfield.getBitfieldLength());
+}
+
+PieceHandle DefaultPieceStorage::getMissingPiece(const SharedHandle<Peer>& peer)
+{
+  return getMissingPiece(peer->getBitfield(), peer->getBitfieldLength());
+}
+
+void DefaultPieceStorage::createFastIndexBitfield
+(BitfieldMan& bitfield, const SharedHandle<Peer>& peer)
+{
+  for(std::deque<size_t>::const_iterator itr =
+	peer->getPeerAllowedIndexSet().begin();
+      itr != peer->getPeerAllowedIndexSet().end(); ++itr) {
+    if(!bitfieldMan->isBitSet(*itr) && peer->hasPiece(*itr)) {
+      bitfield.setBit(*itr);
+    }
+  }
+}
+
+PieceHandle DefaultPieceStorage::getMissingFastPiece
+(const SharedHandle<Peer>& peer)
 {
   if(peer->isFastExtensionEnabled() && peer->countPeerAllowedIndexSet() > 0) {
     BitfieldMan tempBitfield(bitfieldMan->getBlockLength(),
 			     bitfieldMan->getTotalLength());
-    for(std::deque<size_t>::const_iterator itr = peer->getPeerAllowedIndexSet().begin();
+    createFastIndexBitfield(tempBitfield, peer);
-	itr != peer->getPeerAllowedIndexSet().end(); itr++) {
+    return getMissingPiece(tempBitfield);
-      if(!bitfieldMan->isBitSet(*itr) && peer->hasPiece(*itr)) {
+  } else {
-	tempBitfield.setBit(*itr);
+    return SharedHandle<Piece>();
-      }
-    }
-    if(isEndGame()) {
-      return bitfieldMan->getMissingIndex(index, tempBitfield.getBitfield(),
-					  tempBitfield.getBitfieldLength());
-    } else {
-      return bitfieldMan->getMissingUnusedIndex(index,
-						tempBitfield.getBitfield(),
-						tempBitfield.getBitfieldLength());
-    }
   }
-  return false;
 }
 
-PieceHandle DefaultPieceStorage::getMissingFastPiece(const PeerHandle& peer)
+static void unsetExcludedIndexes(BitfieldMan& bitfield,
+				 const std::deque<size_t>& excludedIndexes)
 {
-  size_t index;
+  for(std::deque<size_t>::const_iterator i = excludedIndexes.begin();
-  if(getMissingFastPieceIndex(index, peer)) {
+      i != excludedIndexes.end(); ++i) {
-    return checkOutPiece(index);
+    bitfield.unsetBit(*i);
+  }
+}
+
+SharedHandle<Piece> DefaultPieceStorage::getMissingPiece
+(const SharedHandle<Peer>& peer, const std::deque<size_t>& excludedIndexes)
+{
+  BitfieldMan tempBitfield(bitfieldMan->getBlockLength(),
+			   bitfieldMan->getTotalLength());
+  tempBitfield.setBitfield(peer->getBitfield(), peer->getBitfieldLength());
+  unsetExcludedIndexes(tempBitfield, excludedIndexes);
+  return getMissingPiece(tempBitfield);
+}
+
+SharedHandle<Piece> DefaultPieceStorage::getMissingFastPiece
+(const SharedHandle<Peer>& peer, const std::deque<size_t>& excludedIndexes)
+{
+  if(peer->isFastExtensionEnabled() && peer->countPeerAllowedIndexSet() > 0) {
+    BitfieldMan tempBitfield(bitfieldMan->getBlockLength(),
+			     bitfieldMan->getTotalLength());
+    createFastIndexBitfield(tempBitfield, peer);
+    unsetExcludedIndexes(tempBitfield, excludedIndexes);
+    return getMissingPiece(tempBitfield);
   } else {
     return SharedHandle<Piece>();
   }

src/DefaultPieceStorage.h

@@ -83,8 +83,17 @@ private:
 
   SharedHandle<RarestPieceSelector> _pieceSelector;
 
-  bool getMissingPieceIndex(size_t& index, const SharedHandle<Peer>& peer);
+  bool getMissingPieceIndex(size_t& index,
-  bool getMissingFastPieceIndex(size_t& index, const SharedHandle<Peer>& peer);
+			    const unsigned char* bitfield, size_t& length);
+
+  SharedHandle<Piece> getMissingPiece(const unsigned char* bitfield,
+				      size_t length);
+
+  SharedHandle<Piece> getMissingPiece(const BitfieldMan& bitfield);
+
+  void createFastIndexBitfield(BitfieldMan& bitfield,
+			       const SharedHandle<Peer>& peer);
+
   SharedHandle<Piece> checkOutPiece(size_t index);
 //   size_t deleteUsedPiecesByFillRate(int fillRate, size_t toDelete);
 //   void reduceUsedPieces(size_t upperBound);
@@ -109,6 +118,12 @@ public:
 
   virtual SharedHandle<Piece> getMissingFastPiece(const SharedHandle<Peer>& peer);
 
+  virtual SharedHandle<Piece> getMissingPiece
+  (const SharedHandle<Peer>& peer, const std::deque<size_t>& excludedIndexes);
+
+  virtual SharedHandle<Piece> getMissingFastPiece
+  (const SharedHandle<Peer>& peer, const std::deque<size_t>& excludedIndexes);
+
   virtual SharedHandle<Piece> getMissingPiece();
 
   virtual SharedHandle<Piece> getMissingPiece(size_t index);

src/PieceStorage.h

@@ -67,6 +67,14 @@ public:
   virtual SharedHandle<Piece>
   getMissingPiece(const SharedHandle<Peer>& peer) = 0;
 
+  /**
+   * Same as getMissingPiece(const SharedHandle<Peer>& peer), but the indexes in
+   * excludedIndexes are excluded.
+   */
+  virtual SharedHandle<Piece> getMissingPiece
+  (const SharedHandle<Peer>& peer,
+   const std::deque<size_t>& excludedIndexes) = 0;
+
   /**
    * Returns a piece that the peer has but localhost doesn't.
    * Only pieces that declared as "fast" are returned.
@@ -76,6 +84,14 @@ public:
    */
   virtual SharedHandle<Piece>
   getMissingFastPiece(const SharedHandle<Peer>& peer) = 0;
+  
+  /**
+   * Same as getMissingFastPiece(const SharedHandle<Peer>& peer), but the
+   * indexes in excludedIndexes are excluded.
+   */
+  virtual SharedHandle<Piece> getMissingFastPiece
+  (const SharedHandle<Peer>& peer,
+   const std::deque<size_t>& excludedIndexes) = 0;
 
   /**
    * Returns a missing piece if available. Otherwise returns 0;

src/UnknownLengthPieceStorage.cc

@@ -79,11 +79,23 @@ SharedHandle<Piece> UnknownLengthPieceStorage::getMissingPiece(const SharedHandl
   abort();
 }
 
+SharedHandle<Piece> UnknownLengthPieceStorage::getMissingPiece
+(const SharedHandle<Peer>& peer, const std::deque<size_t>& excludedIndexes)
+{
+  abort();
+}
+
 SharedHandle<Piece> UnknownLengthPieceStorage::getMissingFastPiece(const SharedHandle<Peer>& peer)
 {
   abort();
 }
 
+SharedHandle<Piece> UnknownLengthPieceStorage::getMissingFastPiece
+(const SharedHandle<Peer>& peer, const std::deque<size_t>& excludedIndexes)
+{
+  abort();
+}
+
 PieceHandle UnknownLengthPieceStorage::getMissingPiece()
 {
   if(_downloadFinished) {

src/UnknownLengthPieceStorage.h

@@ -79,6 +79,9 @@ public:
    */
   virtual SharedHandle<Piece> getMissingPiece(const SharedHandle<Peer>& peer);
 
+  virtual SharedHandle<Piece> getMissingPiece
+  (const SharedHandle<Peer>& peer, const std::deque<size_t>& excludedIndexes);
+
   /**
    * Returns a piece that the peer has but localhost doesn't.
    * Only pieces that declared as "fast" are returned.
@@ -88,6 +91,9 @@ public:
    */
   virtual SharedHandle<Piece> getMissingFastPiece(const SharedHandle<Peer>& peer);
 
+  virtual SharedHandle<Piece> getMissingFastPiece
+  (const SharedHandle<Peer>& peer, const std::deque<size_t>& excludedIndexes);
+
   /**
    * Returns a missing piece if available. Otherwise returns 0;
    */

test/DefaultBtRequestFactoryTest.cc

@@ -25,6 +25,7 @@ class DefaultBtRequestFactoryTest:public CppUnit::TestFixture {
   CPPUNIT_TEST(testCreateRequestMessages);
   CPPUNIT_TEST(testCreateRequestMessages_onEndGame);
   CPPUNIT_TEST(testRemoveTargetPiece);
+  CPPUNIT_TEST(testGetTargetPieceIndexes);
   CPPUNIT_TEST_SUITE_END();
 private:
   SharedHandle<DefaultBtRequestFactory> btRequestFactory;
@@ -35,6 +36,7 @@ public:
   void testCreateRequestMessages();
   void testCreateRequestMessages_onEndGame();
   void testRemoveTargetPiece();
+  void testGetTargetPieceIndexes();
 
   class MockBtRequestMessage : public MockBtMessage {
   public:
@@ -234,4 +236,22 @@ void DefaultBtRequestFactoryTest::testRemoveTargetPiece() {
 			   piece1) == btRequestFactory->getTargetPieces().end());
 }
 
+void DefaultBtRequestFactoryTest::testGetTargetPieceIndexes()
+{
+  SharedHandle<Piece> piece1(new Piece(1, btContext->getPieceLength()));
+  SharedHandle<Piece> piece3(new Piece(3, btContext->getPieceLength()));
+  SharedHandle<Piece> piece5(new Piece(5, btContext->getPieceLength()));
+
+  btRequestFactory->addTargetPiece(piece3);
+  btRequestFactory->addTargetPiece(piece1);
+  btRequestFactory->addTargetPiece(piece5);
+
+  std::deque<size_t> indexes;
+  btRequestFactory->getTargetPieceIndexes(indexes);
+  CPPUNIT_ASSERT_EQUAL((size_t)3, indexes.size());
+  CPPUNIT_ASSERT_EQUAL((size_t)3, indexes[0]);
+  CPPUNIT_ASSERT_EQUAL((size_t)1, indexes[1]);
+  CPPUNIT_ASSERT_EQUAL((size_t)5, indexes[2]);
+}
+
 } // namespace aria2

test/DefaultPieceStorageTest.cc

@@ -18,7 +18,9 @@ class DefaultPieceStorageTest:public CppUnit::TestFixture {
   CPPUNIT_TEST_SUITE(DefaultPieceStorageTest);
   CPPUNIT_TEST(testGetTotalLength);
   CPPUNIT_TEST(testGetMissingPiece);
+  CPPUNIT_TEST(testGetMissingPiece_excludedIndexes);
   CPPUNIT_TEST(testGetMissingFastPiece);
+  CPPUNIT_TEST(testGetMissingFastPiece_excludedIndexes);
   CPPUNIT_TEST(testHasMissingPiece);
   CPPUNIT_TEST(testCompletePiece);
   CPPUNIT_TEST(testGetPiece);
@@ -56,7 +58,9 @@ public:
 
   void testGetTotalLength();
   void testGetMissingPiece();
+  void testGetMissingPiece_excludedIndexes();
   void testGetMissingFastPiece();
+  void testGetMissingFastPiece_excludedIndexes();
   void testHasMissingPiece();
   void testCompletePiece();
   void testGetPiece();
@@ -94,6 +98,28 @@ void DefaultPieceStorageTest::testGetMissingPiece() {
   CPPUNIT_ASSERT(piece.isNull());
 }
 
+void DefaultPieceStorageTest::testGetMissingPiece_excludedIndexes()
+{
+  DefaultPieceStorage pss(btContext, option, false);
+  pss.setEndGamePieceNum(0);
+
+  peer->setAllBitfield();
+
+  std::deque<size_t> excludedIndexes;
+  excludedIndexes.push_back(1);
+
+  SharedHandle<Piece> piece = pss.getMissingPiece(peer, excludedIndexes);
+  CPPUNIT_ASSERT_EQUAL(std::string("piece: index=0, length=128"),
+		       piece->toString());
+
+  piece = pss.getMissingPiece(peer, excludedIndexes);
+  CPPUNIT_ASSERT_EQUAL(std::string("piece: index=2, length=128"),
+		       piece->toString());
+
+  piece = pss.getMissingPiece(peer, excludedIndexes);
+  CPPUNIT_ASSERT(piece.isNull());
+}
+
 void DefaultPieceStorageTest::testGetMissingFastPiece() {
   DefaultPieceStorage pss(btContext, option, false);
   pss.setEndGamePieceNum(0);
@@ -105,6 +131,28 @@ void DefaultPieceStorageTest::testGetMissingFastPiece() {
   SharedHandle<Piece> piece = pss.getMissingFastPiece(peer);
   CPPUNIT_ASSERT_EQUAL(std::string("piece: index=2, length=128"),
 		       piece->toString());
+
+  CPPUNIT_ASSERT(pss.getMissingFastPiece(peer).isNull());
+}
+
+void DefaultPieceStorageTest::testGetMissingFastPiece_excludedIndexes()
+{
+  DefaultPieceStorage pss(btContext, option, false);
+  pss.setEndGamePieceNum(0);
+
+  peer->setAllBitfield();
+  peer->setFastExtensionEnabled(true);
+  peer->addPeerAllowedIndex(1);
+  peer->addPeerAllowedIndex(2);
+
+  std::deque<size_t> excludedIndexes;
+  excludedIndexes.push_back(2);
+
+  SharedHandle<Piece> piece = pss.getMissingFastPiece(peer, excludedIndexes);
+  CPPUNIT_ASSERT_EQUAL(std::string("piece: index=1, length=128"),
+		       piece->toString());
+  
+  CPPUNIT_ASSERT(pss.getMissingFastPiece(peer, excludedIndexes).isNull());
 }
 
 void DefaultPieceStorageTest::testHasMissingPiece() {

test/MockBtRequestFactory.h

@@ -28,6 +28,8 @@ public:
 
   virtual void createRequestMessagesOnEndGame
   (std::deque<SharedHandle<BtMessage> >& requests, size_t max) {}
+
+  virtual void getTargetPieceIndexes(std::deque<size_t>& indexes) const {}
 };
 
 } // namespace aria2

test/MockPieceStorage.h

@@ -42,10 +42,22 @@ public:
     return SharedHandle<Piece>(new Piece());
   }
 
+  virtual SharedHandle<Piece> getMissingPiece
+  (const SharedHandle<Peer>& peer, const std::deque<size_t>& excludedIndexes)
+  {
+    return SharedHandle<Piece>(new Piece());
+  }
+
   virtual SharedHandle<Piece> getMissingFastPiece(const SharedHandle<Peer>& peer) {
     return SharedHandle<Piece>(new Piece());
   }
 
+  virtual SharedHandle<Piece> getMissingFastPiece
+  (const SharedHandle<Peer>& peer, const std::deque<size_t>& excludedIndexes)
+  {
+    return SharedHandle<Piece>(new Piece());
+  }
+
   virtual SharedHandle<Piece> getMissingPiece()
   {
     return SharedHandle<Piece>(new Piece());