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 595d98809be9d13b729d4c37e7d41667d551cfff..df23580863a800e41c2f5c2561719822605966a3 100644 (file)
--- 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 c32462b42b57c8bd9c4da0e448e950adb8e6f4ed..b96b965aab2efc3ced2bafbc6eed596ad11e67c0 100644 (file)
--- 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 c30fc8f30323ae52682682e2332c9fcb0e09300a..f4224798bce211f423574aed7c0496ecb1e0958e 100644 (file)
--- 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 52c8c89496bdec9b46bb5de1487b06d48907eed8..7297734ecf892c9e9051d6fcdd20c4edba383fc9 100644 (file)
--- 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 9cc10d7353b7f67842a0b348e0520249d0dd6e1e..e936332a27b6465bbdadd9c9d0cfa3eb79dabc5e 100644 (file)
--- 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 d8d8209c91145f78f2039f84ec1f9fb009df0a5c..4e2a30ef722cd7a45bcf42dc27c84b90da497a1d 100644 (file)
--- a/src/lib/j2k_encoder.cc
+++ b/src/lib/j2k_encoder.cc
@@ -64,14 +64,7 @@ J2KEncoder::J2KEncoder (shared_ptr<const Film> film, shared_ptr<Writer> 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<PlayerVideo> 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 0feda64355fd95cc0e2d18472c7305cd94b6ec86..d960dfaee99f0da96f329cc82a2b97082556f51d 100644 (file)
--- 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 c40178f418a8960914193245f8fbebe557eead97..9eb51aee5bd226ac0facf16285c99b1753fdf01e 100644 (file)
--- 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<Job>

diff --git a/src/lib/update_checker.cc b/src/lib/update_checker.cc
index 82337f92016905420316ad19daa1d1779804ffcf..0fad330fd58ccd1b9942db4c47bf491c9757826a 100644 (file)
--- 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 7d1088276995a242300402684facbf0290e3876f..dc552e6cbcfb08512500ce61c6047121f6104843 100644 (file)
--- 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 a87249fafdd73d637bfc4649e36ea8137e32a0c3..fe49a5b8ec3e056879d33d940d0734e6ac13aef1 100644 (file)
--- 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 562d106396bb193ff8beec53b6280938a4b9c973..5024272abce3eb99cb6805b8fcbadc9a92e24d60 100644 (file)
--- 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;
        }