Merge lp:~raof/mir/remove-eventloop-self-immolation into lp:mir

[Chris Halse Rogers]

Abort with an error message if attempting to destroy SimpleDispatchThread from a dispatch() callback.

This replaces the previous behaviour of detaching the thread and continuing, which caused problems in the tests.
Fixes: https://bugs.launchpad.net/mir/+bug/1441620

Abort with an error message if attempting to destroy SimpleDispatchThread from a dispatch() callback.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2475
http://jenkins.qa.ubuntu.com/job/mir-ci/3489/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2011
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2010
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1961
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1486
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1486/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1961
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1961/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4897
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19528

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/3489/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

=== modified file 'src/common/dispatch/simple_dispatch_thread.cpp'
--- src/common/dispatch/simple_dispatch_thread.cpp	2015-04-08 03:02:37 +0000
+++ src/common/dispatch/simple_dispatch_thread.cpp	2015-04-10 01:53:32 +0000
@@ -18,6 +18,7 @@
 
 #include "mir/dispatch/simple_dispatch_thread.h"
 #include "mir/dispatch/dispatchable.h"
+#include "mir/logging/logger.h"
 #include "utils.h"
 
 #include <sys/epoll.h>
@@ -136,8 +137,13 @@
     if (eventloop.get_id() == std::this_thread::get_id())
     {
         // We're being destroyed from within the dispatch callback
-        // That's OK; we'll exit once we return back to wait_for_events_forever
-        eventloop.detach();
+        // Attempting to join the eventloop will result in a trivial deadlock.
+        // 
+        // The std::thread destructor will call std::terminate() for us, let's
+        // leave a useful message.
+        mir::logging::log(mir::logging::Severity::critical,
+                          "Destroying SimpleDispatchThread from within a dispatch callback. This is a programming error.",
+                          "Dispatch");
     }
     else if (eventloop.joinable())
     {

=== modified file 'tests/unit-tests/dispatch/test_simple_dispatch_thread.cpp'
--- tests/unit-tests/dispatch/test_simple_dispatch_thread.cpp	2015-04-08 03:02:37 +0000
+++ tests/unit-tests/dispatch/test_simple_dispatch_thread.cpp	2015-04-10 01:53:32 +0000
@@ -28,6 +28,7 @@
 #include <fcntl.h>
 
 #include <atomic>
+#include <thread>
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
@@ -192,32 +193,6 @@
     EXPECT_FALSE(dispatched_closed->wait_for(std::chrono::seconds{1}));
 }
 
-TEST_F(SimpleDispatchThreadTest, handles_destruction_from_dispatch_callback)
-{
-    using namespace testing;
-    using namespace std::chrono_literals;
-
-    auto dispatched = std::make_shared<mt::Signal>();
-    auto assignment_made = std::make_shared<mt::Signal>();
-    md::SimpleDispatchThread* dispatcher{nullptr};
-
-    auto dispatchable = std::make_shared<mt::TestDispatchable>([dispatched, &dispatcher, assignment_made]()
-                                                               {
-                                                                   assignment_made->wait_for(10s);
-                                                                   delete dispatcher;
-                                                                   dispatched->raise();
-                                                               });
-
-    dispatchable->trigger();
-    dispatchable->trigger();
-
-    dispatcher = new md::SimpleDispatchThread{dispatchable};
-
-    assignment_made->raise();
-
-    EXPECT_TRUE(dispatched->wait_for(10s));
-}
-
 // Regression test for: lp #1439719
 // The bug involves uninitialized memory and is also sensitive to signal
 // timings, so this test does not always catch the problem. However, repeated
@@ -263,3 +238,23 @@
     auto const result = child->wait_for_termination(10s);
     EXPECT_TRUE(result.succeeded());
 }
+
+using SimpleDispatchThreadDeathTest = SimpleDispatchThreadTest;
+
+TEST_F(SimpleDispatchThreadDeathTest, destroying_dispatcher_from_a_callback_is_an_error)
+{
+    using namespace testing;
+    using namespace std::literals::chrono_literals;
+
+    EXPECT_EXIT(
+    {
+        md::SimpleDispatchThread* dispatcher;
+    
+        auto dispatchable = std::make_shared<mt::TestDispatchable>([&dispatcher]() { delete dispatcher; });
+        
+        dispatchable->trigger();
+        dispatcher = new md::SimpleDispatchThread{dispatchable};
+        std::this_thread::sleep_for(10s);
+    }, KilledBySignal(SIGABRT), ".*Destroying SimpleDispatchThread.*");
+}
+