Merge lp:~afrantzis/mir/fix-1439719-simple-dispatch-thread-eintr into lp:mir

[Alexandros Frantzis]

common: Fix SimpleDispatchThreadTest signal interruption bug (LP: #1439719)

common: Fix SimpleDispatchThreadTest signal interruption bug (LP: #1439719)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2455
http://jenkins.qa.ubuntu.com/job/mir-ci/3447/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1945
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1944/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1895
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1444/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1895
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1895/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4840
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19406

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

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

CI failures are https://bugs.launchpad.net/mir/+bug/1440088 .

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2455
http://jenkins.qa.ubuntu.com/job/mir-ci/3451/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1950
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1949
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1900
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1448
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1448/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1900
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1900/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4846
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19421

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

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

LGTM

review: Approve

[Chris Halse Rogers (raof)]

Ah, it's Linux violating the POSIX mandated behaviour for a small set of system calls, of which epoll_wait happens to be one. Great!

Reading the documentation it seems that this behaviour won't occur on any of our other system calls. Specifically, our calls to readmsg/sendmsg will not error out with EINTR after receiving a SIGSTOP/SIGCONT pair.

LGTM.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1303/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1957
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1956/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1907
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/483
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/483/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1907
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1907/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4853
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19431

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Autolanding failures are https://bugs.launchpad.net/mir/+bug/1440088...

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/common/dispatch/simple_dispatch_thread.cpp'
--- src/common/dispatch/simple_dispatch_thread.cpp	2015-03-31 02:35:42 +0000
+++ src/common/dispatch/simple_dispatch_thread.cpp	2015-04-03 09:11:56 +0000
@@ -23,6 +23,7 @@
 #include <sys/epoll.h>
 #include <unistd.h>
 #include <system_error>
+#include <array>
 #include <signal.h>
 #include <boost/exception/all.hpp>
 
@@ -60,28 +61,43 @@
 
     for (;;)
     {
-        epoll_event events[2];
-        if (epoll_wait(epoll_fd, events, sizeof(events), -1) > 1)
+        std::array<epoll_event,2> events;
+        auto const num_available_events =
+            epoll_wait(epoll_fd, events.data(), events.size(), -1);
+
+        if (num_available_events == 1)
+        {
+            if (events[0].data.u32 == fd_names::dispatchee_fd)
+            {
+                if (!dispatchee->dispatch(md::epoll_to_fd_event(events[0])))
+                {
+                    // No need to keep looping, the Dispatchable's not going to produce any more events.
+                    return;
+                }
+            }
+            else if (events[0].data.u32 == fd_names::shutdown)
+            {
+                // The only thing we do with the shutdown fd is to close it.
+                return;
+            }
+        }
+        else if (num_available_events > 1)
         {
             // Because we only have two fds in the epoll, if there is more than one
             // event pending then one of them must be a shutdown event.
             // So, shutdown.
             return;
         }
-
-        if (events[0].data.u32 == fd_names::dispatchee_fd)
+        else if (num_available_events < 0)
         {
-            if (!dispatchee->dispatch(md::epoll_to_fd_event(events[0])))
+            // Although we have blocked signals in this thread, we can still
+            // get interrupted by SIGSTOP (which is unblockable and non-fatal).
+            if (errno != EINTR)
             {
-                // No need to keep looping, the Dispatchable's not going to produce any more events.
-                return;
+                BOOST_THROW_EXCEPTION((std::system_error{
+                    errno, std::system_category(), "Failed to wait for epoll events"}));
             }
         }
-        else if (events[0].data.u32 == fd_names::shutdown)
-        {
-            // The only thing we do with the shutdown fd is to close it.
-            return;
-        }
     }
 }
 

=== modified file 'tests/include/mir_test/signal.h'
--- tests/include/mir_test/signal.h	2015-03-31 02:35:42 +0000
+++ tests/include/mir_test/signal.h	2015-04-03 09:11:56 +0000
@@ -50,6 +50,8 @@
         return cv.wait_until(lock, time, [this]() { return signalled; });
     }
 
+    void reset();
+
 private:
     std::mutex mutex;
     std::condition_variable cv;

=== modified file 'tests/mir_test/signal.cpp'
--- tests/mir_test/signal.cpp	2015-03-31 02:35:42 +0000
+++ tests/mir_test/signal.cpp	2015-04-03 09:11:56 +0000
@@ -38,3 +38,9 @@
     std::unique_lock<decltype(mutex)> lock(mutex);
     cv.wait(lock, [this]() { return signalled; });
 }
+
+void mt::Signal::reset()
+{
+    std::lock_guard<decltype(mutex)> lock(mutex);
+    signalled = false;
+}

=== modified file 'tests/unit-tests/dispatch/test_simple_dispatch_thread.cpp'
--- tests/unit-tests/dispatch/test_simple_dispatch_thread.cpp	2015-03-31 02:35:42 +0000
+++ tests/unit-tests/dispatch/test_simple_dispatch_thread.cpp	2015-04-03 09:11:56 +0000
@@ -22,6 +22,8 @@
 #include "mir_test/pipe.h"
 #include "mir_test/signal.h"
 #include "mir_test/test_dispatchable.h"
+#include "mir_test_framework/process.h"
+#include "mir_test/cross_process_action.h"
 
 #include <fcntl.h>
 
@@ -215,3 +217,49 @@
 
     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
+// runs (~300, YMMV) consistently fail when run against the problematic code.
+TEST_F(SimpleDispatchThreadTest, keeps_dispatching_after_signal_interruption)
+{
+    using namespace std::chrono_literals;
+    mt::CrossProcessAction stop_and_restart_process;
+
+    auto child = mir_test_framework::fork_and_run_in_a_different_process(
+        [&]
+        {
+            auto dispatched = std::make_shared<mt::Signal>();
+            auto dispatchable = std::make_shared<mt::TestDispatchable>(
+                [dispatched]() { dispatched->raise(); });
+
+            md::SimpleDispatchThread dispatcher{dispatchable};
+            // Ensure the dispatcher has started
+            dispatchable->trigger();
+            EXPECT_TRUE(dispatched->wait_for(1s));
+
+            stop_and_restart_process();
+
+            dispatched->reset();
+            // The dispatcher shouldn't have been affected by the signal
+            dispatchable->trigger();
+            EXPECT_TRUE(dispatched->wait_for(1s));
+            exit(HasFailure() ? EXIT_FAILURE : EXIT_SUCCESS);
+        },
+        []{ return 1; });
+
+    stop_and_restart_process.exec(
+        [child]
+        {
+            // Increase chances of interrupting the dispatch mechanism
+            for (int i = 0; i < 100; ++i)
+            {
+                child->stop();
+                child->cont();
+            }
+        });
+
+    auto const result = child->wait_for_termination(10s);
+    EXPECT_TRUE(result.succeeded());
+}