Merge lp:~raof/mir/threads-and-madness into lp:~mir-team/mir/trunk

[Chris Halse Rogers]

Make libmirclient not aggressively signal-unsafe by blocking signals on our
IO thread.

Fixes infinite loop in XMir (LP: #1199319)

Make libmirclient not aggressively signal-unsafe by blocking signals on our
IO thread.

Fixes: (LP: #1199319)

[Robert Ancell (robert-ancell)]

I don't understand the details but looks sane. Well, except for the commit comment that is...

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:835
http://jenkins.qa.ubuntu.com/job/mir-ci/977/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1263
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1147
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/215
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/215/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/614

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

review: Approve (continuous-integration)

[Thomas Voß (thomas-voss)]

LGTM, and in accordance with http://www.boost.org/doc/libs/1_45_0/doc/html/boost_asio/example/http/server4/posix_main.cpp

review: Approve

=== modified file 'src/client/rpc/mir_socket_rpc_channel.cpp'
--- src/client/rpc/mir_socket_rpc_channel.cpp	2013-07-08 04:51:27 +0000
+++ src/client/rpc/mir_socket_rpc_channel.cpp	2013-07-11 03:13:31 +0000
@@ -26,10 +26,16 @@
 #include "mir_protobuf.pb.h"  // For Buffer frig
 #include "mir_protobuf_wire.pb.h"
 
+#include <boost/exception/errinfo_errno.hpp>
+#include <boost/throw_exception.hpp>
+
 #include <boost/bind.hpp>
 #include <cstring>
 #include <sstream>
 
+#include <pthread.h>
+#include <signal.h>
+
 namespace mclr = mir::client::rpc;
 
 mclr::MirSocketRpcChannel::MirSocketRpcChannel(
@@ -46,8 +52,25 @@
 
     auto run_io_service = boost::bind(&boost::asio::io_service::run, &io_service);
 
+    // Our IO threads must not recieve any signals
+    sigset_t all_signals;
+    sigfillset(&all_signals);
+    sigset_t old_mask;
+    int error;
+    if ((error = pthread_sigmask(SIG_BLOCK, &all_signals, &old_mask)))
+        BOOST_THROW_EXCEPTION(
+            boost::enable_error_info(
+                std::runtime_error("Failed to block signals on IO thread")) << boost::errinfo_errno(error));
+            
+
     io_service_thread = std::move(std::thread(run_io_service));
 
+    // Restore previous signals.
+    if ((error = pthread_sigmask(SIG_SETMASK, &old_mask, NULL)))
+        BOOST_THROW_EXCEPTION(
+            boost::enable_error_info(
+                std::runtime_error("Failed to restore signal mask")) << boost::errinfo_errno(error));
+
     boost::asio::async_read(
         socket,
         boost::asio::buffer(header_bytes),

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2013-06-28 10:42:44 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2013-07-11 03:13:31 +0000
@@ -773,4 +773,93 @@
 
     launch_client_process(client_config);
 }
+
+bool signalled;
+static void SIGIO_handler(int /*signo*/)
+{
+    signalled = true;
+}
+
+TEST_F(DefaultDisplayServerTestFixture, ClientLibraryThreadsHandleNoSignals)
+{
+    struct ClientConfig : ClientConfigCommon
+    {
+        void exec()
+        {
+            signalled = false;
+
+            sigset_t sigset;
+            sigemptyset(&sigset);
+            struct sigaction act;
+            act.sa_handler = &SIGIO_handler;
+            act.sa_mask = sigset;
+            act.sa_flags = 0;
+            act.sa_restorer = nullptr;
+            if (sigaction(SIGIO, &act, NULL))
+                FAIL() << "Failed to set SIGIO action";
+
+            MirConnection* conn = NULL;
+            conn = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
+
+            sigaddset(&sigset, SIGIO);
+            pthread_sigmask(SIG_BLOCK, &sigset, NULL);
+
+            // SIGIO should be blocked
+            if (kill(getpid(), SIGIO))
+                FAIL() << "Failed to send SIGIO signal";
+
+            // Make a roundtrip to the server to ensure the SIGIO has time to be handled
+            MirSurfaceParameters const request_params =
+            {
+                __PRETTY_FUNCTION__,
+                640, 480,
+                mir_pixel_format_abgr_8888,
+                mir_buffer_usage_software
+            };
+
+            surface = mir_connection_create_surface_sync(conn, &request_params);
+
+            mir_connection_release(conn);
+
+            EXPECT_FALSE(signalled);
+        }
+    } client_config;
+
+    launch_client_process(client_config);
+}
+
+TEST_F(DefaultDisplayServerTestFixture, ClientLibraryDoesNotInterfereWithClientSignalHandling)
+{
+    struct ClientConfig : ClientConfigCommon
+    {
+        void exec()
+        {
+            signalled = false;
+
+            sigset_t sigset;
+            sigemptyset(&sigset);
+            struct sigaction act;
+            act.sa_handler = &SIGIO_handler;
+            act.sa_mask = sigset;
+            act.sa_flags = 0;
+            act.sa_restorer = nullptr;
+            if (sigaction(SIGIO, &act, NULL))
+                FAIL() << "Failed to set SIGIO action";
+
+            MirConnection* conn = NULL;
+            conn = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
+
+            // We should receieve SIGIO
+            if (kill(getpid(), SIGIO))
+                FAIL() << "Failed to send SIGIO signal";
+
+            mir_connection_release(conn);
+
+            EXPECT_TRUE(signalled);
+        }
+    } client_config;
+
+    launch_client_process(client_config);
+}
+
 }