diff --git a/include/libtorrent/piece_picker.hpp b/include/libtorrent/piece_picker.hpp index 9221ae6f7..bdbc1d253 100644 --- a/include/libtorrent/piece_picker.hpp +++ b/include/libtorrent/piece_picker.hpp @@ -428,7 +428,8 @@ namespace libtorrent int num_have_filtered() const { return m_num_have_filtered; } // number of pieces whose hash has passed _and_ they have - // been successfully flushed to disk + // been successfully flushed to disk. Including pieces we have + // also filtered with priority 0 but have anyway. int num_have() const { return m_num_have; } // number of pieces whose hash has passed (but haven't necessarily @@ -809,7 +810,8 @@ namespace libtorrent // the number of regions of pieces we don't have. int m_sparse_regions; - // the number of pieces we have (i.e. passed + flushed) + // the number of pieces we have (i.e. passed + flushed). + // This includes pieces that we have filtered but still have int m_num_have; // this is the number of partial download pieces diff --git a/src/piece_picker.cpp b/src/piece_picker.cpp index 4fcad24aa..83e7b6dca 100644 --- a/src/piece_picker.cpp +++ b/src/piece_picker.cpp @@ -1566,6 +1566,9 @@ namespace libtorrent { piece_pos& p = m_piece_map[index]; int download_state = p.download_queue(); + + // this is kind of odd. Could this happen? + TORRENT_ASSERT(download_state != piece_pos::piece_open); if (download_state == piece_pos::piece_open) return; std::vector::iterator i = find_dl_piece(download_state, index); @@ -1590,7 +1593,6 @@ namespace libtorrent TORRENT_ASSERT(index < (int)m_piece_map.size()); piece_pos& p = m_piece_map[index]; - TORRENT_ASSERT(p.downloading() == false); #ifdef TORRENT_PICKER_LOG std::cerr << "[" << this << "] " << "piece_picker::we_dont_have(" << index << ")" << std::endl; @@ -1610,6 +1612,8 @@ namespace libtorrent TORRENT_ASSERT(m_num_passed > 0); --m_num_passed; } + m_downloads[download_state].erase(i); + p.download_state = piece_pos::piece_open; return; } @@ -1661,6 +1665,8 @@ namespace libtorrent int priority = p.priority(this); TORRENT_ASSERT(priority < int(m_priority_boundries.size()) || m_dirty); + if (p.have()) return; + if (p.download_queue() != piece_pos::piece_open) { std::vector::iterator i @@ -1672,8 +1678,6 @@ namespace libtorrent erase_download_piece(i); } - if (p.have()) return; - // maintain sparse_regions if (index == 0) { @@ -3353,6 +3357,8 @@ get_out: // TODO: 2 it would be nice if this could be folded into lock_piece() // the main distinction is that this also maintains the m_num_passed // counter and the passed_hash_check member + // Is there ever a case where we call write filed without also locking + // the piece? Perhaps write_failed() should imply locking it. void piece_picker::write_failed(piece_block block) { TORRENT_PIECE_PICKER_INVARIANT_CHECK; @@ -3470,7 +3476,8 @@ get_out: #endif #ifdef TORRENT_PICKER_LOG - std::cerr << "[" << this << "] " << "mark_as_finished( {" << block.piece_index << ", " << block.block_index << "} )" << std::endl; + std::cerr << "[" << this << "] " << "mark_as_finished( {" + << block.piece_index << ", " << block.block_index << "} )" << std::endl; #endif TORRENT_ASSERT(peer == 0 || static_cast(peer)->in_use); @@ -3562,23 +3569,29 @@ get_out: } + // TODO: 3 this doesn't appear to be used for anything. Can it be removed + // or should it be used to plug some race when requesting and checking pieces? void piece_picker::mark_as_checking(int index) { +/* int state = m_piece_map[index].download_queue(); if (state == piece_pos::piece_open) return; std::vector::iterator i = find_dl_piece(state, index); if (i == m_downloads[state].end()) return; TORRENT_ASSERT(i->outstanding_hash_check == false); i->outstanding_hash_check = true; +*/ } void piece_picker::mark_as_done_checking(int index) { +/* int state = m_piece_map[index].download_queue(); if (state == piece_pos::piece_open) return; std::vector::iterator i = find_dl_piece(state, index); if (i == m_downloads[state].end()) return; i->outstanding_hash_check = false; +*/ } void piece_picker::get_requestors(std::vector& d, int index) const diff --git a/test/test_piece_picker.cpp b/test/test_piece_picker.cpp index 332fca06b..36cd0c02f 100644 --- a/test/test_piece_picker.cpp +++ b/test/test_piece_picker.cpp @@ -302,6 +302,7 @@ int test_main() // test abort_download print_title("test abort_download"); + p = setup_picker("1111111", " ", "7110000", ""); picked = pick_pieces(p, "*******", blocks_per_piece, 0, tmp_peer , options, empty_vector); @@ -357,6 +358,10 @@ int test_main() TEST_CHECK(p->is_requested(piece_block(0, 0)) == false); TEST_CHECK(std::find(picked.begin(), picked.end(), piece_block(0,0)) == picked.end()); +// ======================================================== + + print_title("test abort_download"); + p = setup_picker("1111111", " ", "7110000", ""); p->mark_as_downloading(piece_block(0,0), &tmp1); p->mark_as_finished(piece_block(0,1), 0); @@ -372,6 +377,12 @@ int test_main() TEST_CHECK(p->is_requested(piece_block(0, 0)) == false); TEST_CHECK(std::find(picked.begin(), picked.end(), piece_block(0,0)) != picked.end()); +// ======================================================== + + print_title("test get_downloaders"); + + p = setup_picker("1111111", " ", "7110000", ""); + p->mark_as_downloading(piece_block(0, 2), &tmp1); p->mark_as_writing(piece_block(0, 2), &tmp1); p->abort_download(piece_block(0, 2), &tmp1); @@ -380,7 +391,11 @@ int test_main() std::vector d; p->get_downloaders(d, 0); + TEST_EQUAL(d.size(), 4); + TEST_CHECK(d[0] == NULL); + TEST_CHECK(d[1] == NULL); TEST_CHECK(d[2] == &tmp2); + TEST_CHECK(d[3] == NULL); p->mark_as_downloading(piece_block(0, 3), &tmp1); p->abort_download(piece_block(0, 3), &tmp1); @@ -388,8 +403,25 @@ int test_main() p->mark_as_writing(piece_block(0, 3), &tmp2); p->get_downloaders(d, 0); + + TEST_EQUAL(d.size(), 4); + TEST_CHECK(d[0] == NULL); + TEST_CHECK(d[1] == NULL); + TEST_CHECK(d[2] == &tmp2); TEST_CHECK(d[3] == &tmp2); + // if we ask for downloaders for a piece that's not + // curently being downloaded, we get zeroes back + p->get_downloaders(d, 1); + + TEST_EQUAL(d.size(), 4); + TEST_CHECK(d[0] == NULL); + TEST_CHECK(d[1] == NULL); + TEST_CHECK(d[2] == NULL); + TEST_CHECK(d[3] == NULL); + +// ======================================================== + p = setup_picker("2222", " ", "", ""); for (int i = 0; i < 4; ++i) @@ -1255,18 +1287,15 @@ int test_main() print_availability(p); TEST_CHECK(verify_availability(p, "1111111111111111")); - // make sure it's not dirty pick_pieces(p, "****************", 1, blocks_per_piece, 0); p->dec_refcount(string2vec(" **** ** "), &tmp0); print_availability(p); TEST_CHECK(verify_availability(p, "1100001100111111")); - // make sure it's not dirty pick_pieces(p, "****************", 1, blocks_per_piece, 0); p->inc_refcount(string2vec(" **** ** "), &tmp0); TEST_CHECK(verify_availability(p, "1111111111111111")); - // make sure it's not dirty pick_pieces(p, "****************", 1, blocks_per_piece, 0); p->dec_refcount_all(&tmp0); TEST_CHECK(verify_availability(p, "0000000000000000")); @@ -1275,12 +1304,27 @@ int test_main() print_availability(p); TEST_CHECK(verify_availability(p, "1111111111111111")); - // make sure it's not dirty pick_pieces(p, "****************", 1, blocks_per_piece, 0); p->dec_refcount(3, &tmp1); print_availability(p); TEST_CHECK(verify_availability(p, "1110111111111111")); + p->inc_refcount(string2vec("****************"), &tmp2); + print_availability(p); + TEST_CHECK(verify_availability(p, "2221222222222222")); + + p->inc_refcount(string2vec("* * * * * * * * "), &tmp3); + print_availability(p); + TEST_CHECK(verify_availability(p, "3231323232323232")); + + p->dec_refcount(string2vec("****************"), &tmp2); + print_availability(p); + TEST_CHECK(verify_availability(p, "2120212121212121")); + + p->dec_refcount(string2vec("* * * * * * * * "), &tmp3); + print_availability(p); + TEST_CHECK(verify_availability(p, "1110111111111111")); + // ======================================================== // test reversed peers @@ -1334,8 +1378,142 @@ int test_main() TEST_EQUAL(test_pick(p, piece_picker::rarest_first | piece_picker::reverse), 0); -// MISSING TESTS: -// 2. write_failed +// ======================================================== + + print_title("test piece_stats"); + + p = setup_picker("3456789", "* ", "", "0300000"); + + piece_picker::piece_stats_t stat = p->piece_stats(0); + TEST_EQUAL(stat.peer_count, 3); + TEST_EQUAL(stat.have, 1); + TEST_EQUAL(stat.downloading, 0); + + stat = p->piece_stats(1); + TEST_EQUAL(stat.peer_count, 4); + TEST_EQUAL(stat.have, 0); + TEST_EQUAL(stat.downloading, 1); + +// ======================================================== + + print_title("test piece passed"); + + p = setup_picker("1111111", "* ", "", "0300000"); + + TEST_EQUAL(p->has_piece_passed(0), true); + TEST_EQUAL(p->has_piece_passed(1), false); + TEST_EQUAL(p->num_passed(), 1); + TEST_EQUAL(p->num_have(), 1); + + p->piece_passed(1); + TEST_EQUAL(p->num_passed(), 2); + TEST_EQUAL(p->num_have(), 1); + + p->we_have(1); + TEST_EQUAL(p->num_have(), 2); + + p->mark_as_finished(piece_block(2,0), &tmp1); + p->piece_passed(2); + TEST_EQUAL(p->num_passed(), 3); + // just because the hash check passed doesn't mean + // we "have" the piece. We need to write it to disk first + TEST_EQUAL(p->num_have(), 2); + + // piece 2 already passed the hash check, as soon as we've + // written all the blocks to disk, we should have that piece too + p->mark_as_finished(piece_block(2,1), &tmp1); + p->mark_as_finished(piece_block(2,2), &tmp1); + p->mark_as_finished(piece_block(2,3), &tmp1); + TEST_EQUAL(p->num_have(), 3); + TEST_EQUAL(p->have_piece(2), true); + +// ======================================================== + + print_title("test we dont have"); + + p = setup_picker("1111111", "* * ", "1101111", ""); + TEST_EQUAL(p->has_piece_passed(0), true); + TEST_EQUAL(p->has_piece_passed(1), false); + TEST_EQUAL(p->has_piece_passed(2), true); + TEST_EQUAL(p->num_passed(), 2); + TEST_EQUAL(p->num_have(), 2); + TEST_EQUAL(p->num_have_filtered(), 1); + TEST_EQUAL(p->num_filtered(), 0); + + p->we_dont_have(0); + + TEST_EQUAL(p->has_piece_passed(0), false); + TEST_EQUAL(p->has_piece_passed(1), false); + TEST_EQUAL(p->has_piece_passed(2), true); + TEST_EQUAL(p->num_passed(), 1); + TEST_EQUAL(p->num_have(), 1); + TEST_EQUAL(p->num_have_filtered(), 1); + + p = setup_picker("1111111", "* * ", "1101111", ""); + TEST_EQUAL(p->has_piece_passed(0), true); + TEST_EQUAL(p->has_piece_passed(1), false); + TEST_EQUAL(p->has_piece_passed(2), true); + TEST_EQUAL(p->num_passed(), 2); + TEST_EQUAL(p->num_have(), 2); + TEST_EQUAL(p->num_have_filtered(), 1); + TEST_EQUAL(p->num_filtered(), 0); + + p->we_dont_have(2); + + TEST_EQUAL(p->has_piece_passed(0), true); + TEST_EQUAL(p->has_piece_passed(1), false); + TEST_EQUAL(p->has_piece_passed(2), false); + TEST_EQUAL(p->num_passed(), 1); + TEST_EQUAL(p->num_have(), 1); + TEST_EQUAL(p->num_have_filtered(), 0); + +// ======================================================== + + print_title("test we dont have (don't have but passed hash check)"); + + p = setup_picker("1111111", "* * ", "1101111", "0200000"); + + TEST_EQUAL(p->has_piece_passed(0), true); + TEST_EQUAL(p->has_piece_passed(1), false); + TEST_EQUAL(p->have_piece(0), true) + TEST_EQUAL(p->have_piece(1), false) + + p->piece_passed(1); + + TEST_EQUAL(p->has_piece_passed(0), true); + TEST_EQUAL(p->has_piece_passed(1), true); + TEST_EQUAL(p->have_piece(1), false) + + p->we_dont_have(1); + + TEST_EQUAL(p->has_piece_passed(0), true); + TEST_EQUAL(p->has_piece_passed(1), false); + TEST_EQUAL(p->have_piece(1), false) + +// ======================================================== + + print_title("test write_failed"); + + p = setup_picker("1111111", "* * ", "1101111", "0200000"); + + TEST_EQUAL(p->has_piece_passed(0), true); + TEST_EQUAL(p->has_piece_passed(1), false); + TEST_EQUAL(p->have_piece(1), false); + + p->piece_passed(1); + + TEST_EQUAL(p->has_piece_passed(0), true); + TEST_EQUAL(p->has_piece_passed(1), true); + TEST_EQUAL(p->have_piece(1), false); + + p->mark_as_writing(piece_block(1, 0), &tmp1); + p->write_failed(piece_block(1, 0)); + + TEST_EQUAL(p->has_piece_passed(0), true); + TEST_EQUAL(p->has_piece_passed(1), false); + TEST_EQUAL(p->have_piece(1), false); + +// ======================================================== /*