From: Carl Hetherington Date: Wed, 29 Jul 2020 18:22:54 +0000 (+0200) Subject: Fix bugs in thread termination causing occasional pthread X-Git-Tag: v2.15.94~2 X-Git-Url: https://main.carlh.net/gitweb/?p=dcpomatic.git;a=commitdiff_plain;h=e3c7656f9dc0acbaf518c051b847ee2e4eb7ba23 Fix bugs in thread termination causing occasional pthread assertion failures. Before this, it was possible for J2KEncoder::terminate_threads() to finish without terminating all threads if the thread _running_ terminate_threads() was itself interrupt()ed. This is because the thread_group::join_all() in terminate_threads() is an interruption point, so it was possible it not to complete but instead to throw interrupted_exception. Then the owning J2KEncoder would be torn down but the threads would still be running, causing use-after-frees. This commit adds some boost::this_thread::disable_interruption objects to ensure that the owning thread is not interrupted while it is being destroyed. Also tidy up code that does this stuff, assuming that it's safe to not call thread::joinable but instead do thread.interrupt(); try { thread.join(); } catch (...) {} --- diff --git a/src/lib/butler.cc b/src/lib/butler.cc index 595d98809..df2358086 100644 --- a/src/lib/butler.cc +++ b/src/lib/butler.cc @@ -101,6 +101,8 @@ Butler::Butler ( Butler::~Butler () { + boost::this_thread::disable_interruption dis; + { boost::mutex::scoped_lock lm (_mutex); _stop_thread = true; @@ -113,9 +115,7 @@ Butler::~Butler () _thread.interrupt (); try { _thread.join (); - } catch (...) { - - } + } catch (...) {} } /** Caller must hold a lock on _mutex */ diff --git a/src/lib/checker.cc b/src/lib/checker.cc index c32462b42..b96b965aa 100644 --- a/src/lib/checker.cc +++ b/src/lib/checker.cc @@ -43,19 +43,17 @@ Checker::run () Checker::~Checker () { + boost::this_thread::disable_interruption dis; + { boost::mutex::scoped_lock lm (_mutex); _terminate = true; } - if (_thread.joinable()) { - _thread.interrupt (); - try { - _thread.join (); - } catch (...) { - - } - } + _thread.interrupt (); + try { + _thread.join (); + } catch (...) {} } void diff --git a/src/lib/encode_server.cc b/src/lib/encode_server.cc index c30fc8f30..f4224798b 100644 --- a/src/lib/encode_server.cc +++ b/src/lib/encode_server.cc @@ -83,6 +83,8 @@ EncodeServer::EncodeServer (bool verbose, int num_threads) EncodeServer::~EncodeServer () { + boost::this_thread::disable_interruption dis; + { boost::mutex::scoped_lock lm (_mutex); _terminate = true; @@ -104,13 +106,9 @@ EncodeServer::~EncodeServer () } _broadcast.io_service.stop (); - if (_broadcast.thread.joinable()) { - try { - _broadcast.thread.join (); - } catch (...) { - - } - } + try { + _broadcast.thread.join (); + } catch (...) {} } /** @param after_read Filled in with gettimeofday() after reading the input from the network. diff --git a/src/lib/encode_server_finder.cc b/src/lib/encode_server_finder.cc index 52c8c8949..7297734ec 100644 --- a/src/lib/encode_server_finder.cc +++ b/src/lib/encode_server_finder.cc @@ -70,25 +70,19 @@ EncodeServerFinder::~EncodeServerFinder () void EncodeServerFinder::stop () { + boost::this_thread::disable_interruption dis; + _stop = true; _search_condition.notify_all (); - if (_search_thread.joinable()) { - try { - _search_thread.join(); - } catch (...) { - - } - } + try { + _search_thread.join(); + } catch (...) {} _listen_io_service.stop (); - if (_listen_thread.joinable()) { - try { - _listen_thread.join (); - } catch (...) { - - } - } + try { + _listen_thread.join (); + } catch (...) {} boost::mutex::scoped_lock lm (_servers_mutex); _servers.clear (); diff --git a/src/lib/hints.cc b/src/lib/hints.cc index 9cc10d735..e936332a2 100644 --- a/src/lib/hints.cc +++ b/src/lib/hints.cc @@ -69,17 +69,13 @@ Hints::start () Hints::~Hints () { - if (!_thread.joinable()) { - return; - } + boost::this_thread::disable_interruption dis; try { _stop = true; _thread.interrupt (); _thread.join (); - } catch (...) { - - } + } catch (...) {} } void diff --git a/src/lib/j2k_encoder.cc b/src/lib/j2k_encoder.cc index d8d8209c9..4e2a30ef7 100644 --- a/src/lib/j2k_encoder.cc +++ b/src/lib/j2k_encoder.cc @@ -64,14 +64,7 @@ J2KEncoder::J2KEncoder (shared_ptr film, shared_ptr writer) J2KEncoder::~J2KEncoder () { - try { - terminate_threads (); - } catch (...) { - /* Destructors must not throw exceptions; anything bad - happening now is too late to worry about anyway, - I think. - */ - } + terminate_threads (); } void @@ -246,6 +239,8 @@ J2KEncoder::encode (shared_ptr pv, DCPTime time) void J2KEncoder::terminate_threads () { + boost::this_thread::disable_interruption dis; + if (!_threads) { return; } diff --git a/src/lib/job.cc b/src/lib/job.cc index 0feda6435..d960dfaee 100644 --- a/src/lib/job.cc +++ b/src/lib/job.cc @@ -71,16 +71,12 @@ Job::~Job () void Job::stop_thread () { - if (!_thread.joinable()) { - return; - } + boost::this_thread::disable_interruption dis; _thread.interrupt (); try { _thread.join (); - } catch (...) { - /* Too late to do anything about this */ - } + } catch (...) {} } /** Start the job in a separate thread, returning immediately */ diff --git a/src/lib/job_manager.cc b/src/lib/job_manager.cc index c40178f41..9eb51aee5 100644 --- a/src/lib/job_manager.cc +++ b/src/lib/job_manager.cc @@ -62,6 +62,8 @@ JobManager::start () JobManager::~JobManager () { + boost::this_thread::disable_interruption dis; + BOOST_FOREACH (boost::signals2::connection& i, _connections) { i.disconnect (); } @@ -72,13 +74,9 @@ JobManager::~JobManager () _empty_condition.notify_all (); } - if (_scheduler.joinable()) { - try { - _scheduler.join(); - } catch (...) { - - } - } + try { + _scheduler.join(); + } catch (...) {} } shared_ptr diff --git a/src/lib/update_checker.cc b/src/lib/update_checker.cc index 82337f920..0fad330fd 100644 --- a/src/lib/update_checker.cc +++ b/src/lib/update_checker.cc @@ -81,19 +81,17 @@ UpdateChecker::start () UpdateChecker::~UpdateChecker () { + boost::this_thread::disable_interruption dis; + { boost::mutex::scoped_lock lm (_process_mutex); _terminate = true; } _condition.notify_all (); - if (_thread.joinable()) { - try { - _thread.join (); - } catch (...) { - - } - } + try { + _thread.join (); + } catch (...) {} curl_easy_cleanup (_curl); delete[] _buffer; diff --git a/src/lib/writer.cc b/src/lib/writer.cc index 7d1088276..dc552e6cb 100644 --- a/src/lib/writer.cc +++ b/src/lib/writer.cc @@ -489,17 +489,18 @@ catch (...) void Writer::terminate_thread (bool can_throw) { + boost::this_thread::disable_interruption dis; + boost::mutex::scoped_lock lock (_state_mutex); - if (!_thread.joinable()) { - return; - } _finish = true; _empty_condition.notify_all (); _full_condition.notify_all (); lock.unlock (); - _thread.join (); + try { + _thread.join (); + } catch (...) {} if (can_throw) { rethrow (); diff --git a/src/wx/gl_video_view.cc b/src/wx/gl_video_view.cc index a87249faf..fe49a5b8e 100644 --- a/src/wx/gl_video_view.cc +++ b/src/wx/gl_video_view.cc @@ -104,12 +104,12 @@ GLVideoView::GLVideoView (FilmViewer* viewer, wxWindow *parent) GLVideoView::~GLVideoView () { + boost::this_thread::disable_interruption dis; + try { _thread.interrupt (); _thread.join (); - } catch (...) { - - } + } catch (...) {} glDeleteTextures (1, &_id); } diff --git a/test/socket_test.cc b/test/socket_test.cc index 562d10639..5024272ab 100644 --- a/test/socket_test.cc +++ b/test/socket_test.cc @@ -48,8 +48,11 @@ public: ~TestServer () { + boost::this_thread::disable_interruption dis; stop (); - _thread.join (); + try { + _thread.join (); + } catch (...) {} delete[] _buffer; }