Fix torrent files prioritization (#3133)

After setting file priority, any subsequent attempt to set different priorities will fail if there is a `file_priority` job running in in disk thread. This happens because `torrent::m_file_priority` is being updated before adding disk thread job. The problem is gone if the file priority vector owned by the torrent object, is modified in the `torrent::on_file_priority` callback, when disk job finishes.

ChangeLog

@@ -1,3 +1,5 @@
+	* fix issue when subsequent file priority updates cause torrent to stop
+
 1.1.8 release
 
 	* coalesce reads and writes by default on windows

bindings/python/test.py

@@ -50,6 +50,8 @@ class test_torrent_handle(unittest.TestCase):
 		self.assertEqual(self.h.piece_priorities(), [4])
 
 		self.h.prioritize_files([0,1])
+		# workaround for asynchronous priority update
+		time.sleep(1)
 		self.assertEqual(self.h.file_priorities(), [0,1])
 
 		self.h.prioritize_pieces([0])

src/disk_io_thread.cpp

@@ -2019,6 +2019,8 @@ namespace libtorrent
 		add_job(j);
 	}
 
+	// TODO 3: take `prios` by value and move it into the disk job instead of
+	// copying it into a heap allocation
 	void disk_io_thread::async_set_file_priority(piece_manager* storage
 		, std::vector<boost::uint8_t> const& prios
 		, boost::function<void(disk_io_job const*)> const& handler)

src/torrent.cpp

@@ -1142,17 +1142,12 @@ namespace libtorrent
 #ifndef TORRENT_DISABLE_LOGGING
 		debug_log("*** set-share-mode: %d", s);
 #endif
-
+		if (m_share_mode)
-		// in share mode, all pieces have their priorities initialized to 0
-		if (m_share_mode && valid_metadata())
 		{
-			m_file_priority.clear();
+			int const num_files = valid_metadata() ? m_torrent_file->num_files() : m_file_priority.size();
-			m_file_priority.resize(m_torrent_file->num_files(), 0);
+			// in share mode, all pieces have their priorities initialized to 0
+			prioritize_files(std::vector<int>(num_files));
 		}
-
-		update_piece_priorities();
-
-		if (m_share_mode) recalc_share_mode();
 	}
 
 	void torrent::set_upload_mode(bool b)
@@ -5655,6 +5650,23 @@ namespace {
 
 	namespace
 	{
+		std::vector<boost::uint8_t> fix_priorities(std::vector<int> const& input, file_storage const& fs)
+		{
+			std::vector<boost::uint8_t> files(input.begin(), input.end());
+
+			for (int i = 0; i < std::min<int>(fs.num_files(), files.size()); ++i)
+			{
+				// initialize pad files to priority 0
+				if (files[i] > 0 && fs.pad_file_at(i))
+					files[i] = 0;
+				else if (files[i] > 7)
+					files[i] = 7;
+			}
+
+			files.resize(fs.num_files(), 4);
+			return files;
+		}
+
 		void set_if_greater(int& piece_prio, int file_prio)
 		{
 			if (file_prio > piece_prio) piece_prio = file_prio;
@@ -5664,12 +5676,20 @@ namespace {
 	void torrent::on_file_priority(disk_io_job const* j)
 	{
 		dec_refcount("file_priority");
+
 		boost::scoped_ptr<std::vector<boost::uint8_t> > p(j->buffer.priorities);
-		if (m_file_priority == *p) return;
+
+		if (m_file_priority != *p)
+		{
+			m_file_priority = *p;
+			update_piece_priorities();
+			if (m_share_mode)
+				recalc_share_mode();
+		}
+
+		if (!j->error) return;
 
 		// in this case, some file priorities failed to get set
-		m_file_priority = *p;
-		update_piece_priorities();
 
 		if (alerts().should_post<file_error_alert>())
 			alerts().emplace_alert<file_error_alert>(j->error.ec
@@ -5683,42 +5703,21 @@ namespace {
 	{
 		INVARIANT_CHECK;
 
-		// this call is only valid on torrents with metadata
+		if (is_seed()) return;
-		if (!valid_metadata() || is_seed()) return;
 
-		// the vector need to have exactly one element for every file
+		std::vector<boost::uint8_t> const new_priority = fix_priorities(files, m_torrent_file->files());
-		// in the torrent
-		TORRENT_ASSERT(int(files.size()) == m_torrent_file->num_files());
-
-		int limit = int(files.size());
-		if (valid_metadata() && limit > m_torrent_file->num_files())
-			limit = m_torrent_file->num_files();
-
-		if (int(m_file_priority.size()) < limit)
-			m_file_priority.resize(limit, 4);
-
-		std::copy(files.begin(), files.begin() + limit, m_file_priority.begin());
-
-		if (valid_metadata() && m_torrent_file->num_files() > int(m_file_priority.size()))
-			m_file_priority.resize(m_torrent_file->num_files(), 4);
-
-		// initialize pad files to priority 0
-		file_storage const& fs = m_torrent_file->files();
-		for (int i = 0; i < (std::min)(fs.num_files(), limit); ++i)
-		{
-			if (!fs.pad_file_at(i)) continue;
-			m_file_priority[i] = 0;
-		}
 
 		// storage may be NULL during shutdown
-		if (m_torrent_file->num_pieces() > 0 && m_storage)
+		if (m_storage)
 		{
 			inc_refcount("file_priority");
 			m_ses.disk_thread().async_set_file_priority(m_storage.get()
-				, m_file_priority, boost::bind(&torrent::on_file_priority, shared_from_this(), _1));
+				, new_priority, boost::bind(&torrent::on_file_priority, shared_from_this(), _1));
+		}
+		else
+		{
+			m_file_priority = new_priority;
 		}
-
-		update_piece_priorities();
 	}
 
 	void torrent::set_file_priority(int index, int prio)
@@ -5737,26 +5736,22 @@ namespace {
 
 		if (prio < 0) prio = 0;
 		else if (prio > 7) prio = 7;
-		if (int(m_file_priority.size()) <= index)
-		{
-			// any unallocated slot is assumed to be 4
-			if (prio == 4) return;
-			m_file_priority.resize(index+1, 4);
-		}
 
-		if (m_file_priority[index] == prio) return;
+		std::vector<boost::uint8_t> new_priority = m_file_priority;
-		m_file_priority[index] = prio;
+		new_priority.resize(std::max(int(new_priority.size()), index + 1), 4);
+		new_priority[index] = prio;
 
-		if (!valid_metadata()) return;
+		// storage may be NULL during shutdown
-
-		// stoage may be NULL during shutdown
 		if (m_storage)
 		{
 			inc_refcount("file_priority");
 			m_ses.disk_thread().async_set_file_priority(m_storage.get()
-				, m_file_priority, boost::bind(&torrent::on_file_priority, shared_from_this(), _1));
+				, new_priority, boost::bind(&torrent::on_file_priority, shared_from_this(), _1));
+		}
+		else
+		{
+			m_file_priority = new_priority;
 		}
-		update_piece_priorities();
 	}
 
 	int torrent::file_priority(int index) const
@@ -5784,17 +5779,14 @@ namespace {
 	{
 		INVARIANT_CHECK;
 
+		files->assign(m_file_priority.begin(), m_file_priority.end());
+
 		if (!valid_metadata())
 		{
-			files->resize(m_file_priority.size());
-			std::copy(m_file_priority.begin(), m_file_priority.end(), files->begin());
 			return;
 		}
 
-		files->clear();
 		files->resize(m_torrent_file->num_files(), 4);
-		TORRENT_ASSERT(int(m_file_priority.size()) <= m_torrent_file->num_files());
-		std::copy(m_file_priority.begin(), m_file_priority.end(), files->begin());
 	}
 
 	void torrent::update_piece_priorities()
@@ -7163,8 +7155,6 @@ namespace {
 					m_ses.disk_thread().async_set_file_priority(m_storage.get()
 						, m_file_priority, boost::bind(&torrent::on_file_priority, shared_from_this(), _1));
 				}
-
-				update_piece_priorities();
 			}
 		}
 

test/test_priority.cpp

@@ -394,8 +394,11 @@ TORRENT_TEST(no_metadata_file_prio)
 	torrent_handle h = ses.add_torrent(addp);
 
 	h.file_priority(0, 0);
+	// TODO 2: this should wait for an alert instead of just sleeping
+	test_sleep(100);
 	TEST_EQUAL(h.file_priority(0), 0);
 	h.file_priority(0, 1);
+	test_sleep(100);
 	TEST_EQUAL(h.file_priority(0), 1);
 
 	ses.remove_torrent(h);

test/test_torrent.cpp

@@ -52,10 +52,34 @@ POSSIBILITY OF SUCH DAMAGE.
 using namespace libtorrent;
 namespace lt = libtorrent;
 
+namespace {
+
+bool wait_priority(torrent_handle const& h, std::vector<int> const& prio)
+{
+	for (int i = 0; i < 10; ++i)
+	{
+		if (h.file_priorities() == prio) { return true; }
+
+#ifdef NDEBUG
+		test_sleep(100);
+#else
+		test_sleep(300);
+#endif
+	}
+
+	return h.file_priorities() == prio;
+}
+
+bool prioritize_files(torrent_handle const& h, std::vector<int> const& prio)
+{
+	h.prioritize_files(prio);
+	return wait_priority(h, prio);
+}
+
 void test_running_torrent(boost::shared_ptr<torrent_info> info, boost::int64_t file_size)
 {
 	settings_pack pack = settings();
-	pack.set_int(settings_pack::alert_mask, alert::storage_notification);
+	pack.set_int(settings_pack::alert_mask, alert::progress_notification | alert::storage_notification);
 	pack.set_str(settings_pack::listen_interfaces, "0.0.0.0:48130");
 	pack.set_int(settings_pack::max_retry_port_bind, 10);
 	lt::session ses(pack);
@@ -81,9 +105,9 @@ void test_running_torrent(boost::shared_ptr<torrent_info> info, boost::int64_t f
 	}
 
 	std::vector<int> ones(info->num_files(), 1);
-	h.prioritize_files(ones);
+	TEST_CHECK(prioritize_files(h, ones))
 
-//	test_sleep(500);
+	// test_sleep(500);
 	torrent_status st = h.status();
 
 	TEST_EQUAL(st.total_wanted, file_size); // we want the single file
@@ -91,38 +115,21 @@ void test_running_torrent(boost::shared_ptr<torrent_info> info, boost::int64_t f
 
 	std::vector<int> prio(info->num_files(), 1);
 	prio[0] = 0;
-	h.prioritize_files(prio);
+	TEST_CHECK(prioritize_files(h, prio))
-	st = h.status();
 
+	st = h.status();
 	TEST_EQUAL(st.total_wanted, 0); // we don't want anything
 	TEST_EQUAL(st.total_wanted_done, 0);
 	TEST_EQUAL(int(h.file_priorities().size()), info->num_files());
-	if (!st.is_seeding)
-	{
-		TEST_EQUAL(h.file_priorities()[0], 0);
-		if (info->num_files() > 1)
-			TEST_EQUAL(h.file_priorities()[1], 1);
-		if (info->num_files() > 2)
-			TEST_EQUAL(h.file_priorities()[2], 1);
-	}
 
 	if (info->num_files() > 1)
 	{
 		prio[1] = 0;
-		h.prioritize_files(prio);
+		TEST_CHECK(prioritize_files(h, prio))
-		st = h.status();
 
+		st = h.status();
 		TEST_EQUAL(st.total_wanted, file_size);
 		TEST_EQUAL(st.total_wanted_done, 0);
-		if (!st.is_seeding)
-		{
-			TEST_EQUAL(int(h.file_priorities().size()), info->num_files());
-			TEST_EQUAL(h.file_priorities()[0], 0);
-			if (info->num_files() > 1)
-				TEST_EQUAL(h.file_priorities()[1], 0);
-			if (info->num_files() > 2)
-				TEST_EQUAL(h.file_priorities()[2], 1);
-		}
 	}
 
 	if (info->num_pieces() > 0)
@@ -155,7 +162,10 @@ void test_running_torrent(boost::shared_ptr<torrent_info> info, boost::int64_t f
 			TEST_CHECK(hasher(&piece[0], piece.size()).final() == info->hash_for_piece(0));
 		}
 	}
+
+	TEST_CHECK(h.file_priorities() == prio);
 }
+} // namespace
 
 TORRENT_TEST(long_names)
 {
@@ -214,6 +224,11 @@ TORRENT_TEST(total_wanted)
 	TEST_EQUAL(st.total_wanted, 1024);
 	std::cout << "total_wanted_done: " << st.total_wanted_done << " : 0" << std::endl;
 	TEST_EQUAL(st.total_wanted_done, 0);
+
+	h.file_priority(1, 4);
+	h.file_priority(1, 0);
+	TEST_CHECK(wait_priority(h, std::vector<int>(fs.num_files())));
+	TEST_EQUAL(h.status(0).total_wanted, 0);
 }
 
 TORRENT_TEST(added_peers)
@@ -285,16 +300,16 @@ TORRENT_TEST(torrent)
 		file_storage fs;
 
 		fs.add_file("test_torrent_dir2/tmp1", 1024);
-		libtorrent::create_torrent t(fs, 128 * 1024, 6);
+		libtorrent::create_torrent t(fs, 1024, 6);
 
-		std::vector<char> piece(128 * 1024);
+		std::vector<char> piece(1024);
 		for (int i = 0; i < int(piece.size()); ++i)
 			piece[i] = (i % 26) + 'A';
 
 		// calculate the hash for all pieces
-		sha1_hash ph = hasher(&piece[0], piece.size()).final();
+		sha1_hash const ph = hasher(&piece[0], piece.size()).final();
-		int num = t.num_pieces();
+		int const num = t.num_pieces();
-		TEST_CHECK(t.num_pieces() > 0);
+		TEST_CHECK(num > 0);
 		for (int i = 0; i < num; ++i)
 			t.set_hash(i, ph);
 
@@ -534,4 +549,3 @@ TORRENT_TEST(test_move_storage_no_metadata)
 
 	TEST_EQUAL(h.status().save_path, complete("save_path_1"));
 }
-