2008-11-17 Tatsuhiro Tsujikawa <[email protected]>

	Fixed the bug that causes segmentation fault/bus error during
	executing choking algorithm while seeding. This is caused by
	improper implementation of compare function which returns
	inconsistent results depending on the timing of last unchoke.
	* src/BtSeederStateChoke.cc
	* src/BtSeederStateChoke.h
	* src/DefaultPeerStorage.cc

ChangeLog

@@ -1,3 +1,13 @@
+2008-11-17  Tatsuhiro Tsujikawa  <[email protected]>
+
+	Fixed the bug that causes segmentation fault/bus error during executing
+	choking algorithm while seeding. This is caused by improper
+	implementation of compare function which returns inconsistent results
+	depending on the timing of last unchoke.
+	* src/BtSeederStateChoke.cc
+	* src/BtSeederStateChoke.h
+	* src/DefaultPeerStorage.cc
+
 2008-11-16  Tatsuhiro Tsujikawa  <[email protected]>
 
 	Removed TODO

src/BtSeederStateChoke.cc

@@ -36,109 +36,127 @@
 
 #include <algorithm>
 
-#include "BtContext.h"
 #include "Peer.h"
-#include "BtMessageDispatcher.h"
-#include "BtMessageFactory.h"
-#include "BtRequestFactory.h"
-#include "BtMessageReceiver.h"
-#include "PeerConnection.h"
-#include "ExtensionMessageFactory.h"
 #include "Logger.h"
 #include "LogFactory.h"
-#include "a2time.h"
 #include "SimpleRandomizer.h"
 
 namespace aria2 {
 
-BtSeederStateChoke::BtSeederStateChoke(const SharedHandle<BtContext>& btContext):
-  _btContext(btContext),
+BtSeederStateChoke::BtSeederStateChoke():
   _round(0),
   _lastRound(0),
   _logger(LogFactory::getInstance()) {}
 
 BtSeederStateChoke::~BtSeederStateChoke() {}
 
-class RecentUnchoke {
-private:
-  SharedHandle<BtContext> _btContext;
-
-  const struct timeval _now;
-public:
-  RecentUnchoke(const SharedHandle<BtContext>& btContext,
-		const struct timeval& now):
-    _btContext(btContext), _now(now) {}
-
-  bool operator()(Peer* left, Peer* right) const
-  {
-    size_t leftUpload = left->countOutstandingUpload();
-    size_t rightUpload = right->countOutstandingUpload();
-    if(leftUpload && !rightUpload) {
-      return true;
-    } else if(!leftUpload && rightUpload) {
-      return false;
-    }
-    const int TIME_FRAME = 20;
-    if(!left->getLastAmUnchoking().elapsed(TIME_FRAME) &&
-       left->getLastAmUnchoking().isNewer(right->getLastAmUnchoking())) {
-      return true;
-    } else if(!right->getLastAmUnchoking().elapsed(TIME_FRAME)) {
-      return false;
-    } else {
-      return left->calculateUploadSpeed(_now) > right->calculateUploadSpeed(_now);
-    }
+BtSeederStateChoke::PeerEntry::PeerEntry
+(const SharedHandle<Peer>& peer, const struct timeval& now):
+  _peer(peer),
+  _outstandingUpload(peer->countOutstandingUpload()),
+  _lastAmUnchoking(peer->getLastAmUnchoking()),
+  _recentUnchoking(!_lastAmUnchoking.elapsed(TIME_FRAME)),
+  _uploadSpeed(peer->calculateUploadSpeed(now))
+  {}
+
+bool
+BtSeederStateChoke::PeerEntry::operator<(const PeerEntry& rhs) const
+{
+  if(this->_outstandingUpload && !rhs._outstandingUpload) {
+    return true;
+  } else if(!this->_outstandingUpload && rhs._outstandingUpload) {
+    return false;
   }
-};
-
-class NotInterestedPeer {
-public:
-  bool operator()(const Peer* peer) const
-  {
-    return !peer->peerInterested();
+  if(this->_recentUnchoking &&
+     this->_lastAmUnchoking.isNewer(rhs._lastAmUnchoking)) {
+    return true;
+  } else if(rhs._recentUnchoking) {
+    return false;
+  } else {
+    return this->_uploadSpeed > rhs._uploadSpeed;
   }
-};
+}
 
-void BtSeederStateChoke::unchoke(std::deque<Peer*>& peers)
+SharedHandle<Peer> BtSeederStateChoke::PeerEntry::getPeer() const
 {
-  int count = (_round == 2) ? 4 : 3;
+  return _peer;
+}
 
-  struct timeval now;
-  gettimeofday(&now, 0);
+unsigned int BtSeederStateChoke::PeerEntry::getUploadSpeed() const
+{
+  return _uploadSpeed;
+}
 
-  std::sort(peers.begin(), peers.end(), RecentUnchoke(_btContext, now));
+void BtSeederStateChoke::unchoke
+(std::deque<BtSeederStateChoke::PeerEntry>& peers)
+{
+  int count = (_round == 2) ? 4 : 3;
+
+  std::sort(peers.begin(), peers.end());
 
-  std::deque<Peer*>::iterator r = peers.begin();
+  std::deque<PeerEntry>::iterator r = peers.begin();
   for(; r != peers.end() && count; ++r, --count) {
-    (*r)->chokingRequired(false);
-    _logger->info("RU: %s, ulspd=%u", (*r)->ipaddr.c_str(),
-		  (*r)->calculateUploadSpeed(now));
+    (*r).getPeer()->chokingRequired(false);
+    _logger->info("RU: %s, ulspd=%u", (*r).getPeer()->ipaddr.c_str(),
+		  (*r).getUploadSpeed());
   }
   if(_round == 2 && r != peers.end()) {
     std::random_shuffle(r, peers.end(),
 			*(SimpleRandomizer::getInstance().get()));
-    // TODO Is r invalidated here?
-    (*r)->optUnchoking(true);
-    _logger->info("POU: %s", (*r)->ipaddr.c_str());
+    (*r).getPeer()->optUnchoking(true);
+    _logger->info("POU: %s", (*r).getPeer()->ipaddr.c_str());
   }
 }
 
+class ChokingRequired {
+public:
+  void operator()(const SharedHandle<Peer>& peer) const
+  {
+    peer->chokingRequired(true);
+  }
+};
+
+class GenPeerEntry {
+private:
+  struct timeval _now;
+public:
+  GenPeerEntry()
+  {
+    gettimeofday(&_now, 0);
+  }
+
+  BtSeederStateChoke::PeerEntry operator()(const SharedHandle<Peer>& peer) const
+  {
+    return BtSeederStateChoke::PeerEntry(peer, _now);
+  }
+};
+
+class NotInterestedPeer {
+public:
+  bool operator()(const BtSeederStateChoke::PeerEntry& peerEntry) const
+  {
+    return !peerEntry.getPeer()->peerInterested();
+  }
+};
+
 void
 BtSeederStateChoke::executeChoke(const std::deque<SharedHandle<Peer> >& peerSet)
 {
   _logger->info("Seeder state, %d choke round started", _round);
   _lastRound.reset();
 
-  std::deque<Peer*> peers;
-  std::transform(peerSet.begin(), peerSet.end(), std::back_inserter(peers),
-		 std::mem_fun_ref(&SharedHandle<Peer>::get));
-	      
-  std::for_each(peers.begin(), peers.end(),
-		std::bind2nd(std::mem_fun((void (Peer::*)(bool))&Peer::chokingRequired), true));
+  std::deque<PeerEntry> peerEntries;
 
-  peers.erase(std::remove_if(peers.begin(), peers.end(), NotInterestedPeer()),
-	      peers.end());
+  std::for_each(peerSet.begin(), peerSet.end(), ChokingRequired());
+
+  std::transform(peerSet.begin(), peerSet.end(),
+		 std::back_inserter(peerEntries), GenPeerEntry());
+	      
+  peerEntries.erase(std::remove_if(peerEntries.begin(), peerEntries.end(),
+				   NotInterestedPeer()),
+		    peerEntries.end());
 
-  unchoke(peers);
+  unchoke(peerEntries);
   
   if(++_round == 3) {
     _round = 0;

src/BtSeederStateChoke.h

@@ -36,29 +36,50 @@
 #define _D_BT_SEEDER_STATE_CHOKE_H_
 
 #include "common.h"
+
+#include <deque>
+
 #include "SharedHandle.h"
 #include "TimeA2.h"
-#include <deque>
 
 namespace aria2 {
 
-class BtContext;
 class Peer;
 class Logger;
 
 class BtSeederStateChoke {
 private:
-  SharedHandle<BtContext> _btContext;
-
   int _round;
 
   Time _lastRound;
 
   Logger* _logger;
 
-  void unchoke(std::deque<Peer*>& peers);
+  class PeerEntry {
+  private:
+    SharedHandle<Peer> _peer;
+    size_t _outstandingUpload;
+    Time _lastAmUnchoking;
+    bool _recentUnchoking;
+    unsigned int _uploadSpeed;
+    
+    const static time_t TIME_FRAME = 20;
+  public:
+    PeerEntry(const SharedHandle<Peer>& peer, const struct timeval& now);
+
+    bool operator<(const PeerEntry& rhs) const;
+
+    SharedHandle<Peer> getPeer() const;
+
+    unsigned int getUploadSpeed() const;
+  };
+
+  void unchoke(std::deque<PeerEntry>& peers);
+
+  friend class GenPeerEntry;
+  friend class NotInterestedPeer;
 public:
-  BtSeederStateChoke(const SharedHandle<BtContext>& btContext);
+  BtSeederStateChoke();
 
   ~BtSeederStateChoke();
 

src/DefaultPeerStorage.cc

@@ -57,7 +57,7 @@ DefaultPeerStorage::DefaultPeerStorage(const BtContextHandle& btContext,
   maxPeerListSize(BtRuntime::MAX_PEERS+(BtRuntime::MAX_PEERS >> 2)),
   removedPeerSessionDownloadLength(0),
   removedPeerSessionUploadLength(0),
-  _seederStateChoke(new BtSeederStateChoke(btContext)),
+  _seederStateChoke(new BtSeederStateChoke()),
   _leecherStateChoke(new BtLeecherStateChoke())
 {}