From 6d5a6f05ad6c46194d9ccc4dea5dcef30fa35c3d Mon Sep 17 00:00:00 2001 From: d-komarov Date: Wed, 4 Jul 2018 09:16:33 +0300 Subject: [PATCH] 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 | 2 + bindings/python/test.py | 2 + src/disk_io_thread.cpp | 2 + src/torrent.cpp | 112 ++++++++++++++++++---------------------- test/test_priority.cpp | 3 ++ test/test_torrent.cpp | 74 +++++++++++++++----------- 6 files changed, 104 insertions(+), 91 deletions(-) diff --git a/ChangeLog b/ChangeLog index 179353727..aa860ae26 100644 --- a/ChangeLog +++ b/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 diff --git a/bindings/python/test.py b/bindings/python/test.py index 84d6342ae..8781adfea 100644 --- a/bindings/python/test.py +++ b/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]) diff --git a/src/disk_io_thread.cpp b/src/disk_io_thread.cpp index 373d8e2dc..86493ef7c 100644 --- a/src/disk_io_thread.cpp +++ b/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 const& prios , boost::function const& handler) diff --git a/src/torrent.cpp b/src/torrent.cpp index d7ff04bac..9491e4cb1 100644 --- a/src/torrent.cpp +++ b/src/torrent.cpp @@ -1142,17 +1142,12 @@ namespace libtorrent #ifndef TORRENT_DISABLE_LOGGING debug_log("*** set-share-mode: %d", s); #endif - - // in share mode, all pieces have their priorities initialized to 0 - if (m_share_mode && valid_metadata()) + if (m_share_mode) { - m_file_priority.clear(); - m_file_priority.resize(m_torrent_file->num_files(), 0); + int const num_files = valid_metadata() ? m_torrent_file->num_files() : m_file_priority.size(); + // in share mode, all pieces have their priorities initialized to 0 + prioritize_files(std::vector(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 fix_priorities(std::vector const& input, file_storage const& fs) + { + std::vector files(input.begin(), input.end()); + + for (int i = 0; i < std::min(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 > 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()) alerts().emplace_alert(j->error.ec @@ -5683,42 +5703,21 @@ namespace { { INVARIANT_CHECK; - // this call is only valid on torrents with metadata - if (!valid_metadata() || is_seed()) return; + if (is_seed()) return; - // the vector need to have exactly one element for every file - // 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; - } + std::vector const new_priority = fix_priorities(files, m_torrent_file->files()); // 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; - m_file_priority[index] = prio; + std::vector new_priority = m_file_priority; + new_priority.resize(std::max(int(new_priority.size()), index + 1), 4); + new_priority[index] = prio; - if (!valid_metadata()) return; - - // stoage may be NULL during shutdown + // storage 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(); } } diff --git a/test/test_priority.cpp b/test/test_priority.cpp index 2850301d1..2d4ac4c9b 100644 --- a/test/test_priority.cpp +++ b/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); diff --git a/test/test_torrent.cpp b/test/test_torrent.cpp index 4ac7b2a2b..f7f1eba89 100644 --- a/test/test_torrent.cpp +++ b/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 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 const& prio) +{ + h.prioritize_files(prio); + return wait_priority(h, prio); +} + void test_running_torrent(boost::shared_ptr 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 info, boost::int64_t f } std::vector 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 info, boost::int64_t f std::vector prio(info->num_files(), 1); prio[0] = 0; - h.prioritize_files(prio); - st = h.status(); + TEST_CHECK(prioritize_files(h, prio)) + 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); - st = h.status(); + TEST_CHECK(prioritize_files(h, prio)) + 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 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(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 piece(128 * 1024); + std::vector 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(); - int num = t.num_pieces(); - TEST_CHECK(t.num_pieces() > 0); + sha1_hash const ph = hasher(&piece[0], piece.size()).final(); + int const num = t.num_pieces(); + 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")); } -