From 644f707519f74cafc04925379be0b3f60f3939be Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 17 Nov 2008 11:07:04 +0000 Subject: [PATCH] 2008-11-17 Tatsuhiro Tsujikawa 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 | 10 +++ src/BtSeederStateChoke.cc | 158 +++++++++++++++++++++----------------- src/BtSeederStateChoke.h | 33 ++++++-- src/DefaultPeerStorage.cc | 2 +- 4 files changed, 126 insertions(+), 77 deletions(-) diff --git a/ChangeLog b/ChangeLog index 75a1bc87..23bce9be 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2008-11-17 Tatsuhiro Tsujikawa + + 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 Removed TODO diff --git a/src/BtSeederStateChoke.cc b/src/BtSeederStateChoke.cc index 539df5e8..2330f2e9 100644 --- a/src/BtSeederStateChoke.cc +++ b/src/BtSeederStateChoke.cc @@ -36,109 +36,127 @@ #include -#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), +BtSeederStateChoke::BtSeederStateChoke(): _round(0), _lastRound(0), _logger(LogFactory::getInstance()) {} BtSeederStateChoke::~BtSeederStateChoke() {} -class RecentUnchoke { -private: - SharedHandle _btContext; +BtSeederStateChoke::PeerEntry::PeerEntry +(const SharedHandle& peer, const struct timeval& now): + _peer(peer), + _outstandingUpload(peer->countOutstandingUpload()), + _lastAmUnchoking(peer->getLastAmUnchoking()), + _recentUnchoking(!_lastAmUnchoking.elapsed(TIME_FRAME)), + _uploadSpeed(peer->calculateUploadSpeed(now)) + {} - const struct timeval _now; +bool +BtSeederStateChoke::PeerEntry::operator<(const PeerEntry& rhs) const +{ + if(this->_outstandingUpload && !rhs._outstandingUpload) { + return true; + } else if(!this->_outstandingUpload && rhs._outstandingUpload) { + return false; + } + if(this->_recentUnchoking && + this->_lastAmUnchoking.isNewer(rhs._lastAmUnchoking)) { + return true; + } else if(rhs._recentUnchoking) { + return false; + } else { + return this->_uploadSpeed > rhs._uploadSpeed; + } +} + +SharedHandle BtSeederStateChoke::PeerEntry::getPeer() const +{ + return _peer; +} + +unsigned int BtSeederStateChoke::PeerEntry::getUploadSpeed() const +{ + return _uploadSpeed; +} + +void BtSeederStateChoke::unchoke +(std::deque& peers) +{ + int count = (_round == 2) ? 4 : 3; + + std::sort(peers.begin(), peers.end()); + + std::deque::iterator r = peers.begin(); + for(; r != peers.end() && count; ++r, --count) { + (*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())); + (*r).getPeer()->optUnchoking(true); + _logger->info("POU: %s", (*r).getPeer()->ipaddr.c_str()); + } +} + +class ChokingRequired { public: - RecentUnchoke(const SharedHandle& btContext, - const struct timeval& now): - _btContext(btContext), _now(now) {} - - bool operator()(Peer* left, Peer* right) const + void operator()(const SharedHandle& peer) 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); - } + peer->chokingRequired(true); + } +}; + +class GenPeerEntry { +private: + struct timeval _now; +public: + GenPeerEntry() + { + gettimeofday(&_now, 0); + } + + BtSeederStateChoke::PeerEntry operator()(const SharedHandle& peer) const + { + return BtSeederStateChoke::PeerEntry(peer, _now); } }; class NotInterestedPeer { public: - bool operator()(const Peer* peer) const + bool operator()(const BtSeederStateChoke::PeerEntry& peerEntry) const { - return !peer->peerInterested(); + return !peerEntry.getPeer()->peerInterested(); } }; -void BtSeederStateChoke::unchoke(std::deque& peers) -{ - int count = (_round == 2) ? 4 : 3; - - struct timeval now; - gettimeofday(&now, 0); - - std::sort(peers.begin(), peers.end(), RecentUnchoke(_btContext, now)); - - std::deque::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)); - } - 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()); - } -} - void BtSeederStateChoke::executeChoke(const std::deque >& peerSet) { _logger->info("Seeder state, %d choke round started", _round); _lastRound.reset(); - std::deque peers; - std::transform(peerSet.begin(), peerSet.end(), std::back_inserter(peers), - std::mem_fun_ref(&SharedHandle::get)); + std::deque peerEntries; + + std::for_each(peerSet.begin(), peerSet.end(), ChokingRequired()); + + std::transform(peerSet.begin(), peerSet.end(), + std::back_inserter(peerEntries), GenPeerEntry()); - std::for_each(peers.begin(), peers.end(), - std::bind2nd(std::mem_fun((void (Peer::*)(bool))&Peer::chokingRequired), true)); + peerEntries.erase(std::remove_if(peerEntries.begin(), peerEntries.end(), + NotInterestedPeer()), + peerEntries.end()); - peers.erase(std::remove_if(peers.begin(), peers.end(), NotInterestedPeer()), - peers.end()); - - unchoke(peers); + unchoke(peerEntries); if(++_round == 3) { _round = 0; diff --git a/src/BtSeederStateChoke.h b/src/BtSeederStateChoke.h index 79b06075..ef6630b3 100644 --- a/src/BtSeederStateChoke.h +++ b/src/BtSeederStateChoke.h @@ -36,29 +36,50 @@ #define _D_BT_SEEDER_STATE_CHOKE_H_ #include "common.h" + +#include + #include "SharedHandle.h" #include "TimeA2.h" -#include namespace aria2 { -class BtContext; class Peer; class Logger; class BtSeederStateChoke { private: - SharedHandle _btContext; - int _round; Time _lastRound; Logger* _logger; - void unchoke(std::deque& peers); + class PeerEntry { + private: + SharedHandle _peer; + size_t _outstandingUpload; + Time _lastAmUnchoking; + bool _recentUnchoking; + unsigned int _uploadSpeed; + + const static time_t TIME_FRAME = 20; + public: + PeerEntry(const SharedHandle& peer, const struct timeval& now); + + bool operator<(const PeerEntry& rhs) const; + + SharedHandle getPeer() const; + + unsigned int getUploadSpeed() const; + }; + + void unchoke(std::deque& peers); + + friend class GenPeerEntry; + friend class NotInterestedPeer; public: - BtSeederStateChoke(const SharedHandle& btContext); + BtSeederStateChoke(); ~BtSeederStateChoke(); diff --git a/src/DefaultPeerStorage.cc b/src/DefaultPeerStorage.cc index 6e6044c2..9588e1c0 100644 --- a/src/DefaultPeerStorage.cc +++ b/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()) {}