extend piece_picker unit test

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

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<downloading_piece>::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<downloading_piece>::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<torrent_peer*>(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<downloading_piece>::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<downloading_piece>::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<void*>& d, int index) const

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<void*> 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);
+
+// ========================================================
 
 /*