From e45124fc22a12f66dbd6ec593358bef739060a35 Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Sun, 6 Jan 2013 04:02:29 +0000 Subject: [PATCH] back-port heavy weight refcount invariant checking from libtorrent_aio (disabled by default). and also backport piece-picker refcounting bug fix --- include/libtorrent/piece_picker.hpp | 25 ++++++++---- include/libtorrent/torrent.hpp | 28 ++++++------- src/peer_connection.cpp | 22 +++++----- src/piece_picker.cpp | 63 ++++++++++++++++++++++++++--- src/torrent.cpp | 10 ++--- test/test_piece_picker.cpp | 55 +++++++++++++------------ 6 files changed, 134 insertions(+), 69 deletions(-) diff --git a/include/libtorrent/piece_picker.hpp b/include/libtorrent/piece_picker.hpp index a427070a1..36451da83 100644 --- a/include/libtorrent/piece_picker.hpp +++ b/include/libtorrent/piece_picker.hpp @@ -53,6 +53,12 @@ POSSIBILITY OF SUCH DAMAGE. #include "libtorrent/assert.hpp" #include "libtorrent/time.hpp" +// #define TORRENT_DEBUG_REFCOUNTS + +#ifdef TORRENT_DEBUG_REFCOUNTS +#include +#endif + namespace libtorrent { @@ -177,21 +183,21 @@ namespace libtorrent // increases the peer count for the given piece // (is used when a HAVE message is received) - void inc_refcount(int index); - void dec_refcount(int index); + void inc_refcount(int index, const void* peer); + void dec_refcount(int index, const void* peer); // increases the peer count for the given piece // (is used when a BITFIELD message is received) - void inc_refcount(bitfield const& bitmask); + void inc_refcount(bitfield const& bitmask, const void* peer); // decreases the peer count for the given piece // (used when a peer disconnects) - void dec_refcount(bitfield const& bitmask); + void dec_refcount(bitfield const& bitmask, const void* peer); // these will increase and decrease the peer count // of all pieces. They are used when seeds join // or leave the swarm. - void inc_refcount_all(); - void dec_refcount_all(); + void inc_refcount_all(const void* peer); + void dec_refcount_all(const void* peer); // This indicates that we just received this piece // it means that the refcounter will indicate that @@ -429,6 +435,10 @@ namespace libtorrent boost::uint32_t index; #endif +#ifdef TORRENT_DEBUG_REFCOUNTS + // all the peers that have this piece + std::set have_peers; +#endif enum { // index is set to this to indicate that we have the @@ -498,15 +508,16 @@ namespace libtorrent bool operator==(piece_pos p) const { return index == p.index && peer_count == p.peer_count; } - }; private: +#ifndef TORRENT_DEBUG_REFCOUNTS #if TORRENT_COMPACT_PICKER BOOST_STATIC_ASSERT(sizeof(piece_pos) == sizeof(char) * 4); #else BOOST_STATIC_ASSERT(sizeof(piece_pos) == sizeof(char) * 8); +#endif #endif void break_one_seed(); diff --git a/include/libtorrent/torrent.hpp b/include/libtorrent/torrent.hpp index 0d14988ae..9c4feaff9 100644 --- a/include/libtorrent/torrent.hpp +++ b/include/libtorrent/torrent.hpp @@ -533,11 +533,11 @@ namespace libtorrent } // when we get a have message, this is called for that piece - void peer_has(int index) + void peer_has(int index, peer_connection const* peer) { if (has_picker()) { - m_picker->inc_refcount(index); + m_picker->inc_refcount(index, peer); } #ifdef TORRENT_DEBUG else @@ -548,14 +548,14 @@ namespace libtorrent } // when we get a bitfield message, this is called for that piece - void peer_has(bitfield const& bits) + void peer_has(bitfield const& bits, peer_connection const* peer) { if (has_picker()) { - if (bits.all_set()) - m_picker->inc_refcount_all(); + if (bits.all_set() && bits.size() > 0) + m_picker->inc_refcount_all(peer); else - m_picker->inc_refcount(bits); + m_picker->inc_refcount(bits, peer); } #ifdef TORRENT_DEBUG else @@ -565,11 +565,11 @@ namespace libtorrent #endif } - void peer_has_all() + void peer_has_all(peer_connection const* peer) { if (has_picker()) { - m_picker->inc_refcount_all(); + m_picker->inc_refcount_all(peer); } #ifdef TORRENT_DEBUG else @@ -579,14 +579,14 @@ namespace libtorrent #endif } - void peer_lost(bitfield const& bits) + void peer_lost(bitfield const& bits, peer_connection const* peer) { if (has_picker()) { - if (bits.all_set()) - m_picker->dec_refcount_all(); + if (bits.all_set() && bits.size() > 0) + m_picker->dec_refcount_all(peer); else - m_picker->dec_refcount(bits); + m_picker->dec_refcount(bits, peer); } #ifdef TORRENT_DEBUG else @@ -596,11 +596,11 @@ namespace libtorrent #endif } - void peer_lost(int index) + void peer_lost(int index, peer_connection const* peer) { if (has_picker()) { - m_picker->dec_refcount(index); + m_picker->dec_refcount(index, peer); } #ifdef TORRENT_DEBUG else diff --git a/src/peer_connection.cpp b/src/peer_connection.cpp index 4f89810b7..ba786de18 100644 --- a/src/peer_connection.cpp +++ b/src/peer_connection.cpp @@ -697,7 +697,7 @@ namespace libtorrent t->get_policy().set_seed(m_peer_info, true); m_upload_only = true; - t->peer_has_all(); + t->peer_has_all(this); if (t->is_upload_only()) send_not_interested(); else t->get_policy().peer_is_interesting(*this); return; @@ -706,7 +706,7 @@ namespace libtorrent // if we're a seed, we don't keep track of piece availability if (!t->is_seed()) { - t->peer_has(m_have_piece); + t->peer_has(m_have_piece, this); bool interesting = false; for (int i = 0; i < int(m_have_piece.size()); ++i) { @@ -1617,7 +1617,7 @@ namespace libtorrent // we won't have a piece picker) if (!t->valid_metadata()) return; - t->peer_has(index); + t->peer_has(index, this); // this will disregard all have messages we get within // the first two seconds. Since some clients implements @@ -1722,7 +1722,7 @@ namespace libtorrent // we won't have a piece picker) if (!t->valid_metadata()) return; - t->peer_lost(index); + t->peer_lost(index, this); if (was_seed) t->get_policy().set_seed(m_peer_info, false); @@ -1771,7 +1771,7 @@ namespace libtorrent // if we've already received a bitfield message // we first need to count down all the pieces // we believe the peer has first - t->peer_lost(m_have_piece); + t->peer_lost(m_have_piece, this); } m_bitfield_received = true; @@ -1806,7 +1806,7 @@ namespace libtorrent m_have_piece.set_all(); m_num_pieces = num_pieces; - t->peer_has_all(); + t->peer_has_all(this); if (!t->is_upload_only()) t->get_policy().peer_is_interesting(*this); @@ -1819,7 +1819,7 @@ namespace libtorrent // peer has // if we're a seed, we don't keep track of piece availability bool interesting = false; - t->peer_has(bits); + t->peer_has(bits, this); m_have_piece = bits; m_num_pieces = num_pieces; @@ -2625,7 +2625,7 @@ namespace libtorrent if (is_disconnecting()) return; if (m_bitfield_received) - t->peer_lost(m_have_piece); + t->peer_lost(m_have_piece, this); m_have_all = true; @@ -2658,7 +2658,7 @@ namespace libtorrent m_have_piece.set_all(); m_num_pieces = m_have_piece.size(); - t->peer_has_all(); + t->peer_has_all(this); // if we're finished, we're not interested if (t->is_upload_only()) send_not_interested(); @@ -2692,7 +2692,7 @@ namespace libtorrent if (is_disconnecting()) return; if (m_bitfield_received) - t->peer_lost(m_have_piece); + t->peer_lost(m_have_piece, this); t->get_policy().set_seed(m_peer_info, false); m_bitfield_received = true; @@ -5711,7 +5711,7 @@ namespace libtorrent TORRENT_ASSERT(m_disconnect_started); } - if (!m_disconnect_started && m_initialized) + if (!m_disconnect_started && m_initialized && m_ses.settings().close_redundant_connections) { // none of this matters if we're disconnecting anyway if (t->is_upload_only()) diff --git a/src/piece_picker.cpp b/src/piece_picker.cpp index 847b9b7b9..cd681a52f 100644 --- a/src/piece_picker.cpp +++ b/src/piece_picker.cpp @@ -111,6 +111,9 @@ namespace libtorrent i->peer_count = 0; i->downloading = 0; i->index = 0; +#ifdef TORRENT_DEBUG_REFCOUNTS + i->have_peers.clear(); +#endif } for (std::vector::iterator i = m_piece_map.begin() + m_cursor @@ -273,10 +276,12 @@ namespace libtorrent void piece_picker::check_invariant(const torrent* t) const { +#ifndef TORRENT_DEBUG_REFCOUNTS #if TORRENT_COMPACT_PICKER TORRENT_ASSERT(sizeof(piece_pos) == 4); #else TORRENT_ASSERT(sizeof(piece_pos) == 8); +#endif #endif TORRENT_ASSERT(m_num_have >= 0); TORRENT_ASSERT(m_num_have_filtered >= 0); @@ -396,6 +401,11 @@ namespace libtorrent else ++num_have_filtered; } + +#ifdef TORRENT_DEBUG_REFCOUNTS + TORRENT_ASSERT(p.have_peers.size() == p.peer_count + m_seeds); +#endif + if (p.index == piece_pos::we_have_index) ++num_have; @@ -850,7 +860,7 @@ namespace libtorrent else update(prev_priority, p.index); } - void piece_picker::inc_refcount_all() + void piece_picker::inc_refcount_all(const void* peer) { #ifdef TORRENT_EXPENSIVE_INVARIANT_CHECKS TORRENT_PIECE_PICKER_INVARIANT_CHECK; @@ -864,9 +874,18 @@ namespace libtorrent // didn't have any peers m_dirty = true; } + +#ifdef TORRENT_DEBUG_REFCOUNTS + for (std::vector::iterator i = m_piece_map.begin() + , end(m_piece_map.end()); i != end; ++i) + { + TORRENT_ASSERT(i->have_peers.count(peer) == 0); + i->have_peers.insert(peer); + } +#endif } - void piece_picker::dec_refcount_all() + void piece_picker::dec_refcount_all(const void* peer) { #ifdef TORRENT_EXPENSIVE_INVARIANT_CHECKS TORRENT_PIECE_PICKER_INVARIANT_CHECK; @@ -882,6 +901,14 @@ namespace libtorrent // didn't have any peers m_dirty = true; } +#ifdef TORRENT_DEBUG_REFCOUNTS + for (std::vector::iterator i = m_piece_map.begin() + , end(m_piece_map.end()); i != end; ++i) + { + TORRENT_ASSERT(i->have_peers.count(peer) == 1); + i->have_peers.erase(peer); + } +#endif return; } TORRENT_ASSERT(m_seeds == 0); @@ -889,6 +916,11 @@ namespace libtorrent for (std::vector::iterator i = m_piece_map.begin() , end(m_piece_map.end()); i != end; ++i) { +#ifdef TORRENT_DEBUG_REFCOUNTS + TORRENT_ASSERT(i->have_peers.count(peer) == 1); + i->have_peers.erase(peer); +#endif + TORRENT_ASSERT(i->peer_count > 0); --i->peer_count; } @@ -896,7 +928,7 @@ namespace libtorrent m_dirty = true; } - void piece_picker::inc_refcount(int index) + void piece_picker::inc_refcount(int index, const void* peer) { #ifdef TORRENT_EXPENSIVE_INVARIANT_CHECKS TORRENT_PIECE_PICKER_INVARIANT_CHECK; @@ -904,6 +936,11 @@ namespace libtorrent piece_pos& p = m_piece_map[index]; +#ifdef TORRENT_DEBUG_REFCOUNTS + TORRENT_ASSERT(p.have_peers.count(peer) == 0); + p.have_peers.insert(peer); +#endif + int prev_priority = p.priority(this); ++p.peer_count; if (m_dirty) return; @@ -937,7 +974,7 @@ namespace libtorrent m_dirty = true; } - void piece_picker::dec_refcount(int index) + void piece_picker::dec_refcount(int index, const void* peer) { #ifdef TORRENT_EXPENSIVE_INVARIANT_CHECKS TORRENT_PIECE_PICKER_INVARIANT_CHECK; @@ -945,6 +982,11 @@ namespace libtorrent piece_pos& p = m_piece_map[index]; +#ifdef TORRENT_DEBUG_REFCOUNTS + TORRENT_ASSERT(p.have_peers.count(peer) == 1); + p.have_peers.erase(peer); +#endif + if (p.peer_count == 0) { TORRENT_ASSERT(m_seeds > 0); @@ -962,7 +1004,7 @@ namespace libtorrent if (prev_priority >= 0) update(prev_priority, p.index); } - void piece_picker::inc_refcount(bitfield const& bitmask) + void piece_picker::inc_refcount(bitfield const& bitmask, const void* peer) { #ifdef TORRENT_EXPENSIVE_INVARIANT_CHECKS TORRENT_PIECE_PICKER_INVARIANT_CHECK; @@ -976,6 +1018,11 @@ namespace libtorrent { if (*i) { +#ifdef TORRENT_DEBUG_REFCOUNTS + TORRENT_ASSERT(m_piece_map[index].have_peers.count(peer) == 0); + m_piece_map[index].have_peers.insert(peer); +#endif + ++m_piece_map[index].peer_count; updated = true; } @@ -984,7 +1031,7 @@ namespace libtorrent if (updated) m_dirty = true; } - void piece_picker::dec_refcount(bitfield const& bitmask) + void piece_picker::dec_refcount(bitfield const& bitmask, const void* peer) { #ifdef TORRENT_EXPENSIVE_INVARIANT_CHECKS TORRENT_PIECE_PICKER_INVARIANT_CHECK; @@ -1001,6 +1048,10 @@ namespace libtorrent { if (*i) { +#ifdef TORRENT_DEBUG_REFCOUNTS + TORRENT_ASSERT(m_piece_map[index].have_peers.count(peer) == 1); + m_piece_map[index].have_peers.erase(peer); +#endif piece_pos& p = m_piece_map[index]; if (p.peer_count == 0) diff --git a/src/torrent.cpp b/src/torrent.cpp index b20be387e..5f3e5c296 100644 --- a/src/torrent.cpp +++ b/src/torrent.cpp @@ -1163,7 +1163,7 @@ namespace libtorrent peer_request p; p.piece = piece; p.start = 0; - picker().inc_refcount(piece); + picker().inc_refcount(piece, 0); for (int i = 0; i < blocks_in_piece; ++i, p.start += block_size()) { if (picker().is_finished(piece_block(piece, i)) @@ -1175,7 +1175,7 @@ namespace libtorrent // out of memory if (buffer == 0) { - picker().dec_refcount(piece); + picker().dec_refcount(piece, 0); return; } disk_buffer_holder holder(m_ses, buffer); @@ -1188,7 +1188,7 @@ namespace libtorrent } async_verify_piece(piece, boost::bind(&torrent::piece_finished , shared_from_this(), piece, _1)); - picker().dec_refcount(piece); + picker().dec_refcount(piece, 0); } void torrent::on_disk_write_complete(int ret, disk_io_job const& j @@ -4335,7 +4335,7 @@ namespace libtorrent { if (m_picker.get()) { - m_picker->dec_refcount_all(); + m_picker->dec_refcount_all(p); } } else @@ -4344,7 +4344,7 @@ namespace libtorrent { bitfield const& pieces = p->get_bitfield(); TORRENT_ASSERT(pieces.count() <= int(pieces.size())); - m_picker->dec_refcount(pieces); + m_picker->dec_refcount(pieces, p); } } } diff --git a/test/test_piece_picker.cpp b/test/test_piece_picker.cpp index 4070e710a..280dc00f1 100644 --- a/test/test_piece_picker.cpp +++ b/test/test_piece_picker.cpp @@ -56,6 +56,8 @@ bitfield string2vec(char const* have_str) return have; } +policy::peer* tmp_peer = 0; + // availability is a string where each character is the // availability of that piece, '1', '2' etc. // have_str is a string where each character represents a @@ -78,7 +80,7 @@ boost::shared_ptr setup_picker( const int avail = availability[i] - '0'; assert(avail >= 0); - for (int j = 0; j < avail; ++j) p->inc_refcount(i); + for (int j = 0; j < avail; ++j) p->inc_refcount(i, 0); } bitfield have = string2vec(have_str); @@ -101,16 +103,16 @@ boost::shared_ptr setup_picker( TEST_CHECK(!p->is_finished(piece_block(i, j))); if ((blocks & (1 << j)) == 0) continue; ++counter; - bool ret = p->mark_as_downloading(piece_block(i, j), 0, piece_picker::slow); + bool ret = p->mark_as_downloading(piece_block(i, j), (void*)tmp_peer, piece_picker::slow); TEST_CHECK(ret == true); TEST_CHECK(p->is_requested(piece_block(i, j)) == bool(blocks & (1 << j))); - p->mark_as_writing(piece_block(i, j), 0); + p->mark_as_writing(piece_block(i, j), (void*)tmp_peer); TEST_CHECK(!p->is_finished(piece_block(i, j))); // trying to mark a block as requested after it has been completed // should fail (return false) - ret = p->mark_as_downloading(piece_block(i, j), 0, piece_picker::slow); + ret = p->mark_as_downloading(piece_block(i, j), (void*)tmp_peer, piece_picker::slow); TEST_CHECK(ret == false); - p->mark_as_finished(piece_block(i, j), 0); + p->mark_as_finished(piece_block(i, j), (void*)tmp_peer); TEST_CHECK(p->is_downloaded(piece_block(i, j)) == bool(blocks & (1 << j))); TEST_CHECK(p->is_finished(piece_block(i, j)) == bool(blocks & (1 << j))); @@ -258,6 +260,7 @@ int test_main() tmp3.in_use = true; peer_struct.in_use = true; #endif + tmp_peer = &tmp1; std::vector picked; boost::shared_ptr p; const std::vector empty_vector; @@ -801,15 +804,15 @@ int test_main() dc = p->distributed_copies(); std::cout << "distributed copies: " << dc.first << "." << (dc.second / 1000.f) << std::endl; TEST_CHECK(dc == std::make_pair(1, 5000 / 7)); - p->inc_refcount_all(); + p->inc_refcount_all(0); dc = p->distributed_copies(); TEST_CHECK(dc == std::make_pair(2, 5000 / 7)); - p->dec_refcount_all(); + p->dec_refcount_all(0); dc = p->distributed_copies(); std::cout << "distributed copies: " << dc.first << "." << (dc.second / 1000.f) << std::endl; TEST_CHECK(dc == std::make_pair(1, 5000 / 7)); - p->inc_refcount(0); - p->dec_refcount_all(); + p->inc_refcount(0, 0); + p->dec_refcount_all(0); dc = p->distributed_copies(); std::cout << "distributed copies: " << dc.first << "." << (dc.second / 1000.f) << std::endl; TEST_CHECK(dc == std::make_pair(0, 6000 / 7)); @@ -823,7 +826,7 @@ int test_main() dc = p->distributed_copies(); std::cout << "distributed copies: " << dc.first << "." << (dc.second / 1000.f) << std::endl; TEST_CHECK(dc == std::make_pair(1, 5000 / 7)); - p->inc_refcount_all(); + p->inc_refcount_all(0); dc = p->distributed_copies(); std::cout << "distributed copies: " << dc.first << "." << (dc.second / 1000.f) << std::endl; TEST_CHECK(dc == std::make_pair(2, 5000 / 7)); @@ -836,24 +839,24 @@ int test_main() p = setup_picker("1233333", " * ", "", ""); TEST_CHECK(test_pick(p) == 0); - p->dec_refcount(0); + p->dec_refcount(0, 0); TEST_CHECK(test_pick(p) == 1); - p->dec_refcount(4); - p->dec_refcount(4); + p->dec_refcount(4, 0); + p->dec_refcount(4, 0); TEST_CHECK(test_pick(p) == 4); // decrease refcount on something that's not in the piece list - p->dec_refcount(5); - p->inc_refcount(5); + p->dec_refcount(5, 0); + p->inc_refcount(5, 0); bitfield bits(7); bits.clear_all(); bits.set_bit(0); - p->inc_refcount(bits); + p->inc_refcount(bits, 0); bits.clear_all(); bits.set_bit(4); - p->dec_refcount(bits); + p->dec_refcount(bits, 0); TEST_CHECK(test_pick(p) == 0); // ======================================================== @@ -862,9 +865,9 @@ int test_main() print_title("test unverified blocks"); p = setup_picker("1111111", " ", "", "0300700"); TEST_CHECK(p->unverified_blocks() == 2 + 3); - TEST_CHECK(p->get_downloader(piece_block(4, 0)) == 0); - TEST_CHECK(p->get_downloader(piece_block(4, 1)) == 0); - TEST_CHECK(p->get_downloader(piece_block(4, 2)) == 0); + TEST_CHECK(p->get_downloader(piece_block(4, 0)) == (void*)tmp_peer); + TEST_CHECK(p->get_downloader(piece_block(4, 1)) == (void*)tmp_peer); + TEST_CHECK(p->get_downloader(piece_block(4, 2)) == (void*)tmp_peer); TEST_CHECK(p->get_downloader(piece_block(4, 3)) == 0); p->mark_as_downloading(piece_block(4, 3), &peer_struct, piece_picker::fast); TEST_CHECK(p->get_downloader(piece_block(4, 3)) == &peer_struct); @@ -988,33 +991,33 @@ int test_main() // make sure it's not dirty pick_pieces(p, "****************", 1, 1, 0, piece_picker::fast, options, empty_vector); - p->inc_refcount_all(); + p->inc_refcount_all((void*)2); print_availability(p); TEST_CHECK(verify_availability(p, "1111111111111111")); // make sure it's not dirty pick_pieces(p, "****************", 1, 1, 0, piece_picker::fast, options, empty_vector); - p->dec_refcount(string2vec(" **** ** ")); + p->dec_refcount(string2vec(" **** ** "), (void*)4); print_availability(p); TEST_CHECK(verify_availability(p, "1100001100111111")); // make sure it's not dirty pick_pieces(p, "****************", 1, 1, 0, piece_picker::fast, options, empty_vector); - p->inc_refcount(string2vec(" **** ** ")); + p->inc_refcount(string2vec(" **** ** "), (void*)5); TEST_CHECK(verify_availability(p, "1111111111111111")); // make sure it's not dirty pick_pieces(p, "****************", 1, 1, 0, piece_picker::fast, options, empty_vector); - p->dec_refcount_all(); + p->dec_refcount_all((void*)2); TEST_CHECK(verify_availability(p, "0000000000000000")); - p->inc_refcount_all(); + p->inc_refcount_all((void*)2); print_availability(p); TEST_CHECK(verify_availability(p, "1111111111111111")); // make sure it's not dirty pick_pieces(p, "****************", 1, 1, 0, piece_picker::fast, options, empty_vector); - p->dec_refcount(3); + p->dec_refcount(3, (void*)4); print_availability(p); TEST_CHECK(verify_availability(p, "1110111111111111"));