fix issue of rapid calls to file_priority() clobbering each other

ChangeLog

@@ -1,3 +1,4 @@
+	* fix issue of rapid calls to file_priority() clobbering each other
 	* clear tracker errors on success
 	* optimize setting with unlimited unchoke slots
 	* fixed restoring of trackers, comment, creation date and created-by in resume data

include/libtorrent/torrent.hpp

@@ -575,7 +575,7 @@ namespace libtorrent {
 		download_priority_t file_priority(file_index_t index) const;
 
 		void on_file_priority(storage_error const& err, aux::vector<download_priority_t, file_index_t> prios);
-		void prioritize_files(aux::vector<download_priority_t, file_index_t> const& files);
+		void prioritize_files(aux::vector<download_priority_t, file_index_t> files);
 		void file_priorities(aux::vector<download_priority_t, file_index_t>*) const;
 
 		void cancel_non_critical();
@@ -1273,6 +1273,12 @@ namespace libtorrent {
 		// TODO: this wastes 5 bits per file
 		aux::vector<download_priority_t, file_index_t> m_file_priority;
 
+		// any file priority updateds attempted while another file priority update
+		// in in-progress/outstanding with the disk I/O thread, are queued up in
+		// this dictionary. Once the outstanding update comes back, all of these
+		// are applied in one batch
+		std::map<file_index_t, download_priority_t> m_deferred_file_priorities;
+
 		// this object is used to track download progress of individual files
 		aux::file_progress m_file_progress;
 
@@ -1704,6 +1710,9 @@ namespace libtorrent {
 		// the torrent error
 		bool m_torrent_initialized:1;
 
+		// this is set to true while waiting for an async_set_file_priority
+		bool m_outstanding_file_priority:1;
+
 #if TORRENT_USE_ASSERTS
 		// set to true when torrent is start()ed. It may only be started once
 		bool m_was_started = false;

src/torrent.cpp

@@ -227,6 +227,7 @@ bool is_downloading_state(int const st)
 		, m_downloaded(0xffffff)
 		, m_progress_ppm(0)
 		, m_torrent_initialized(false)
+		, m_outstanding_file_priority(false)
 	{
 		// we cannot log in the constructor, because it relies on shared_from_this
 		// being initialized, which happens after the constructor returns.
@@ -4963,29 +4964,28 @@ bool is_downloading_state(int const st)
 	namespace
 	{
 		aux::vector<download_priority_t, file_index_t> fix_priorities(
-			aux::vector<download_priority_t, file_index_t> const& input
+			aux::vector<download_priority_t, file_index_t> input
 			, file_storage const* fs)
 		{
-			aux::vector<download_priority_t, file_index_t> files(input.begin(), input.end());
+			if (fs) input.resize(fs->num_files(), default_priority);
 
-			if (fs) files.resize(fs->num_files(), default_priority);
-
-			for (file_index_t i : files.range())
+			for (file_index_t i : input.range())
 			{
 				// initialize pad files to priority 0
-				if (files[i] > dont_download && fs && fs->pad_file_at(i))
-					files[i] = dont_download;
-				else if (files[i] > top_priority)
-					files[i] = top_priority;
+				if (input[i] > dont_download && fs && fs->pad_file_at(i))
+					input[i] = dont_download;
+				else if (input[i] > top_priority)
+					input[i] = top_priority;
 			}
 
-			return files;
+			return input;
 		}
 	}
 
 	void torrent::on_file_priority(storage_error const& err
 		, aux::vector<download_priority_t, file_index_t> prios)
 	{
+		m_outstanding_file_priority = false;
 		COMPLETE_ASYNC("file_priority");
 		if (m_file_priority != prios)
 		{
@@ -4994,23 +4994,43 @@ bool is_downloading_state(int const st)
 				recalc_share_mode();
 		}
 
-		if (!err) return;
+		if (err)
+		{
+			// in this case, some file priorities failed to get set
+			if (alerts().should_post<file_error_alert>())
+				alerts().emplace_alert<file_error_alert>(err.ec
+					, resolve_filename(err.file()), err.operation, get_handle());
 
-		// in this case, some file priorities failed to get set
-
-		if (alerts().should_post<file_error_alert>())
-			alerts().emplace_alert<file_error_alert>(err.ec
-				, resolve_filename(err.file()), err.operation, get_handle());
-
-		set_error(err.ec, err.file());
-		pause();
+			set_error(err.ec, err.file());
+			pause();
+		}
+		else if (!m_deferred_file_priorities.empty() && !m_abort)
+		{
+			auto new_priority = m_file_priority;
+			// resize the vector if we have to. The last item in the map has the
+			// highest file index.
+			auto const max_idx = std::prev(m_deferred_file_priorities.end())->first;
+			if (new_priority.end_index() <= max_idx)
+			{
+				// any unallocated slot is assumed to have the default priority
+				new_priority.resize(static_cast<int>(max_idx) + 1, default_priority);
+			}
+			for (auto const& p : m_deferred_file_priorities)
+			{
+				file_index_t const index = p.first;
+				download_priority_t const prio = p.second;
+				new_priority[index] = prio;
+			}
+			m_deferred_file_priorities.clear();
+			prioritize_files(std::move(new_priority));
+		}
 	}
 
-	void torrent::prioritize_files(aux::vector<download_priority_t, file_index_t> const& files)
+	void torrent::prioritize_files(aux::vector<download_priority_t, file_index_t> files)
 	{
 		INVARIANT_CHECK;
 
-		auto new_priority = fix_priorities(files
+		auto new_priority = fix_priorities(std::move(files)
 			, valid_metadata() ? &m_torrent_file->files() : nullptr);
 
 		// storage may be NULL during shutdown
@@ -5023,6 +5043,7 @@ bool is_downloading_state(int const st)
 			// possibly not fully updated.
 			update_piece_priorities(new_priority);
 
+			m_outstanding_file_priority = true;
 			ADD_OUTSTANDING_ASYNC("file_priority");
 			m_ses.disk_thread().async_set_file_priority(m_storage
 				, std::move(new_priority), std::bind(&torrent::on_file_priority, shared_from_this(), _1, _2));
@@ -5048,6 +5069,13 @@ bool is_downloading_state(int const st)
 		}
 
 		prio = aux::clamp(prio, dont_download, top_priority);
+
+		if (m_outstanding_file_priority)
+		{
+			m_deferred_file_priorities[index] = prio;
+			return;
+		}
+
 		auto new_priority = m_file_priority;
 		if (new_priority.end_index() <= index)
 		{
@@ -5066,6 +5094,7 @@ bool is_downloading_state(int const st)
 			// piece priorities still stay the same, but the file priorities are
 			// possibly not fully updated.
 			update_piece_priorities(new_priority);
+			m_outstanding_file_priority = true;
 			ADD_OUTSTANDING_ASYNC("file_priority");
 			m_ses.disk_thread().async_set_file_priority(m_storage
 				, std::move(new_priority), std::bind(&torrent::on_file_priority, shared_from_this(), _1, _2));

src/torrent_handle.cpp

@@ -520,6 +520,7 @@ namespace libtorrent {
 		return sync_call_ret<download_priority_t>(dont_download, &torrent::file_priority, index);
 	}
 
+	// TODO: support moving files into this call
 	void torrent_handle::prioritize_files(std::vector<download_priority_t> const& files) const
 	{
 		async_call(&torrent::prioritize_files

test/test_priority.cpp

@@ -484,6 +484,35 @@ TORRENT_TEST(no_metadata_piece_prio)
 	ses.remove_torrent(h);
 }
 
+TORRENT_TEST(file_priority_multiple_calls)
+{
+	settings_pack pack = settings();
+	lt::session ses(pack);
+
+	error_code ec;
+	auto t = ::generate_torrent(true);
+
+	add_torrent_params addp;
+	addp.flags &= ~torrent_flags::paused;
+	addp.flags &= ~torrent_flags::auto_managed;
+	addp.save_path = ".";
+	addp.ti = t;
+	torrent_handle h = ses.add_torrent(addp);
+
+	for (file_index_t const i : t->files().file_range())
+		h.file_priority(i, lt::low_priority);
+
+	std::vector<download_priority_t> const expected(
+		std::size_t(t->files().num_files()), lt::low_priority);
+	for (int i = 0; i < 10; ++i)
+	{
+		auto const p = h.get_file_priorities();
+		if (p == expected) return;
+		std::this_thread::sleep_for(milliseconds(500));
+	}
+	TEST_CHECK(false);
+}
+
 TORRENT_TEST(export_file_while_seed)
 {
 	settings_pack pack = settings();

test/test_torrent.cpp

@@ -254,7 +254,6 @@ TORRENT_TEST(total_wanted)
 	// the last set priority
 	h.file_priority(file_index_t{1}, default_priority);
 	h.file_priority(file_index_t{1}, dont_download);
-	TEST_EQUAL(h.status({}).total_wanted, 0);
 	TEST_CHECK(wait_priority(h, aux::vector<download_priority_t, file_index_t>(static_cast<std::size_t>(fs.num_files()))));
 	TEST_EQUAL(h.status({}).total_wanted, 0);
 }