From 062e1796c025d729d49b7dbcec9e7127f12489be Mon Sep 17 00:00:00 2001 From: Arvid Norberg Date: Wed, 13 May 2009 17:17:33 +0000 Subject: [PATCH] fixed crash when shutting down while checking a torrent --- ChangeLog | 1 + include/libtorrent/disk_io_thread.hpp | 12 ++++++++ src/disk_io_thread.cpp | 8 ++++- test/test_storage.cpp | 43 +++++++++++++++++---------- 4 files changed, 47 insertions(+), 17 deletions(-) diff --git a/ChangeLog b/ChangeLog index 260b7ef4f..fb77521c9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -46,6 +46,7 @@ release 0.14.4 in an error state which would pause the torrent * fixed case when move_storage() would fail. Added a new alert to be posted when it does + * fixed crash bug when shutting down while checking a torrent release 0.14.3 diff --git a/include/libtorrent/disk_io_thread.hpp b/include/libtorrent/disk_io_thread.hpp index 67b0c5641..327a66d00 100644 --- a/include/libtorrent/disk_io_thread.hpp +++ b/include/libtorrent/disk_io_thread.hpp @@ -337,6 +337,18 @@ namespace libtorrent io_service& m_ios; + // this keeps the io_service::run() call blocked from + // returning. When shutting down, it's possible that + // the event queue is drained before the disk_io_thread + // has posted its last callback. When this happens, the + // io_service will have a pending callback from the + // disk_io_thread, but the event loop is not running. + // this means that the event is destructed after the + // disk_io_thread. If the event refers to a disk buffer + // it will try to free it, but the buffer pool won't + // exist anymore, and crash. This prevents that. + boost::optional m_work; + // thread for performing blocking disk io operations boost::thread m_disk_io_thread; }; diff --git a/src/disk_io_thread.cpp b/src/disk_io_thread.cpp index 60168a82d..3812ee388 100644 --- a/src/disk_io_thread.cpp +++ b/src/disk_io_thread.cpp @@ -238,6 +238,7 @@ namespace libtorrent , m_abort(false) , m_queue_buffer_size(0) , m_ios(ios) + , m_work(io_service::work(m_ios)) , m_disk_io_thread(boost::ref(*this)) { #ifdef TORRENT_DISK_STATS @@ -974,6 +975,7 @@ namespace libtorrent void disk_io_thread::add_job(disk_io_job const& j , boost::function const& f) { + TORRENT_ASSERT(!m_abort); TORRENT_ASSERT(!j.callback); TORRENT_ASSERT(j.storage || j.action == disk_io_job::abort_thread @@ -1090,6 +1092,9 @@ namespace libtorrent free_piece(*i, l); m_pieces.clear(); m_read_pieces.clear(); + // release the io_service to allow the run() call to return + // we do this once we stop posting new callbacks to it. + m_work.reset(); return; } @@ -1170,7 +1175,6 @@ namespace libtorrent m_log << log_time() << " abort_thread " << std::endl; #endif mutex_t::scoped_lock jl(m_queue_mutex); - m_abort = true; for (std::list::iterator i = m_jobs.begin(); i != m_jobs.end();) @@ -1189,6 +1193,8 @@ namespace libtorrent } ++i; } + + m_abort = true; break; } case disk_io_job::read_and_hash: diff --git a/test/test_storage.cpp b/test/test_storage.cpp index ecfe88ea3..16dd6efe0 100644 --- a/test/test_storage.cpp +++ b/test/test_storage.cpp @@ -64,7 +64,7 @@ void on_read_piece(int ret, disk_io_job const& j, char const* data, int size) if (ret > 0) TEST_CHECK(std::equal(j.buffer, j.buffer + ret, data)); } -void on_check_resume_data(int ret, disk_io_job const& j) +void on_check_resume_data(int ret, disk_io_job const& j, bool* done) { std::cerr << "on_check_resume_data ret: " << ret; switch (ret) @@ -75,19 +75,20 @@ void on_check_resume_data(int ret, disk_io_job const& j) << " file: " << j.error_file << std::endl; break; case -3: std::cerr << " aborted" << std::endl; break; } + *done = true; } -void on_check_files(int ret, disk_io_job const& j) +void on_check_files(int ret, disk_io_job const& j, bool* done) { std::cerr << "on_check_files ret: " << ret; switch (ret) { - case 0: std::cerr << " done" << std::endl; break; + case 0: std::cerr << " done" << std::endl; *done = true; break; case -1: std::cerr << " current slot: " << j.piece << " have: " << j.offset << std::endl; break; case -2: std::cerr << " disk error: " << j.str - << " file: " << j.error_file << std::endl; break; - case -3: std::cerr << " aborted" << std::endl; break; + << " file: " << j.error_file << std::endl; *done = true; break; + case -3: std::cerr << " aborted" << std::endl; *done = true; break; } } @@ -187,19 +188,23 @@ void run_storage_tests(boost::intrusive_ptr info boost::mutex lock; error_code ec; + bool done = false; lazy_entry frd; - pm->async_check_fastresume(&frd, &on_check_resume_data); + pm->async_check_fastresume(&frd, boost::bind(&on_check_resume_data, _1, _2, &done)); ios.reset(); - ios.run(ec); - - pm->async_check_files(&on_check_files); - for (int i = 0; i < 4; ++i) + while (!done) + { + ios.reset(); + ios.run_one(ec); + } + + done = false; + pm->async_check_files(boost::bind(&on_check_files, _1, _2, &done)); + while (!done) { ios.reset(); ios.run_one(ec); } - ios.reset(); - ios.poll(ec); // test rename_file remove(test_path / "part0"); @@ -260,7 +265,8 @@ void run_storage_tests(boost::intrusive_ptr info TEST_CHECK(!exists(test_path / "part0")); TEST_CHECK(exists(test_path / "temp_storage/test1.tmp")); - ios.run(ec); + ios.reset(); + ios.poll(ec); io.join(); remove_all(test_path / "temp_storage2"); @@ -367,13 +373,18 @@ void test_check_files(path const& test_path boost::mutex lock; error_code ec; + bool done = false; lazy_entry frd; - pm->async_check_fastresume(&frd, &on_check_resume_data); + pm->async_check_fastresume(&frd, boost::bind(&on_check_resume_data, _1, _2, &done)); ios.reset(); - ios.run(ec); + while (!done) + { + ios.reset(); + ios.run_one(ec); + } bool pieces[4] = {false, false, false, false}; - bool done = false; + done = false; pm->async_check_files(bind(&check_files_fill_array, _1, _2, pieces, &done)); while (!done)