Merge lp:~alan-griffiths/mir/fix-1201436 into lp:mir

[Alan Griffiths]

client: Fix race in connection failures

client: Fix race in connection failures

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1097
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/62/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2190
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2075
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/62
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/62/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/62/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1099
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/65/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2193
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2078
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/65
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/65/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/65/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

nit:
+ // We don't die while if are pending callbacks (as they touch this).

lgtm otherwise

review: Approve

[Daniel van Vugt (vanvugt)]

You need to retest your conditions after a wait returns. So:
    if (received < expecting)
        wait_condition.wait_for(lock, limit);
should be:
    while (received < expecting)
        wait_condition.wait_for(lock, limit);

This is for two reasons:
  1. Some other thread could be waiting too, and racing against you. So you need to recheck the condition via a while loop after the wait returns (and when you have regained the lock) to verify the condition is really met, and you didn't lose a race.
  2. Historically OS wait functions could return prematurely (eg signals interrupting) so needed sanity checking. But I don't think this would still be true in the C++11 implementation... ?

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1100
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/68/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2196
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2081
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/68
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/68/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/68/rebuild

review: Approve (continuous-integration)

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-09-11 21:20:15 +0000
+++ src/client/mir_client_library.cpp	2013-10-02 08:59:32 +0000
@@ -97,9 +97,10 @@
 
         mcl::DefaultConnectionConfiguration conf{sock};
 
-        MirConnection* connection = new MirConnection(conf);
-
-        return connection->connect(name, callback, context);
+        std::unique_ptr<MirConnection> connection{new MirConnection(conf)};
+        auto const result = connection->connect(name, callback, context);
+        connection.release();
+        return result;
     }
     catch (std::exception const& x)
     {
 
=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2013-09-27 13:40:30 +0000
+++ src/client/mir_connection.cpp	2013-10-02 08:59:32 +0000
@@ -63,7 +63,6 @@
 
 }
 
-
 MirConnection::MirConnection() :
     channel(),
     server(0),
@@ -91,6 +90,10 @@
 
 MirConnection::~MirConnection() noexcept
 {
+    // We don't die while if are pending callbacks (as they touch this).
+    // But, if after 500ms we don't get a call, assume it won't happen.
+    connect_wait_handle.wait_for_pending(std::chrono::milliseconds(500));
+
     std::lock_guard<std::mutex> lock(connection_guard);
     valid_connections.erase(this);
 
@@ -200,6 +203,7 @@
 
 void MirConnection::connected(mir_connected_callback callback, void * context)
 {
+    bool safe_to_callback = true;
     {
         std::lock_guard<std::recursive_mutex> lock(mutex);
 
@@ -207,6 +211,9 @@
         {
             if (!connect_result.has_error())
             {
+                // We're handling an error scenario that means we're not sync'd
+                // with the client code - a callback isn't safe (or needed)
+                safe_to_callback = false;
                 set_error_message("Connect failed");
             }
         }
@@ -220,7 +227,7 @@
         display_configuration->set_configuration(connect_result.display_configuration());
     }
 
-    callback(this, context);
+    if (safe_to_callback) callback(this, context);
     connect_wait_handle.result_received();
 }
 
@@ -232,6 +239,7 @@
     std::lock_guard<std::recursive_mutex> lock(mutex);
 
     connect_parameters.set_application_name(app_name);
+    connect_wait_handle.expect_result();
     server.connect(
         0,
         &connect_parameters,
 
=== modified file 'src/client/mir_wait_handle.cpp'
--- src/client/mir_wait_handle.cpp	2013-08-28 03:41:48 +0000
+++ src/client/mir_wait_handle.cpp	2013-10-02 08:59:32 +0000
@@ -57,6 +57,19 @@
     expecting = 0;
 }
 
+void MirWaitHandle::wait_for_pending(std::chrono::milliseconds limit)
+{
+    using std::chrono::steady_clock;
+
+    std::unique_lock<std::mutex> lock(guard);
+
+    auto time_limit = steady_clock::now() + limit;
+
+    while (received < expecting && steady_clock::now() < time_limit)
+        wait_condition.wait_until(lock, time_limit);
+}
+
+
 void MirWaitHandle::wait_for_one()  // wait for any single result
 {
     std::unique_lock<std::mutex> lock(guard);
 
=== modified file 'src/client/mir_wait_handle.h'
--- src/client/mir_wait_handle.h	2013-08-28 03:41:48 +0000
+++ src/client/mir_wait_handle.h	2013-10-02 08:59:32 +0000
@@ -20,6 +20,7 @@
 #ifndef MIR_CLIENT_MIR_WAIT_HANDLE_H_
 #define MIR_CLIENT_MIR_WAIT_HANDLE_H_
 
+#include <chrono>
 #include <condition_variable>
 #include <mutex>
 
@@ -37,6 +38,7 @@
     void result_received();
     void wait_for_all();
     void wait_for_one();
+    void wait_for_pending(std::chrono::milliseconds limit);
 
 private:
     std::mutex guard;
 
=== modified file 'src/client/rpc/mir_socket_rpc_channel.cpp'
--- src/client/rpc/mir_socket_rpc_channel.cpp	2013-09-30 09:20:06 +0000
+++ src/client/rpc/mir_socket_rpc_channel.cpp	2013-10-02 08:59:32 +0000
@@ -82,6 +82,14 @@
     init();
 }
 
+void mclr::MirSocketRpcChannel::notify_disconnected()
+{
+    // TODO enable configuring the kill mechanism
+    io_service.stop();
+    raise (SIGTERM);
+    pending_calls.force_completion();
+}
+
 void mclr::MirSocketRpcChannel::init()
 {
     io_service_thread = std::thread([&]
@@ -110,10 +118,7 @@
             {
                 rpc_report->connection_failure(x);
 
-                // TODO enable configuring the kill mechanism
-                io_service.stop();
-                raise(SIGTERM);
-                pending_calls.force_completion();
+                notify_disconnected();
             }
         });
 }
 
=== modified file 'src/client/rpc/mir_socket_rpc_channel.h'
--- src/client/rpc/mir_socket_rpc_channel.h	2013-09-20 18:42:07 +0000
+++ src/client/rpc/mir_socket_rpc_channel.h	2013-10-02 08:59:32 +0000
@@ -93,6 +93,7 @@
     size_t read_message_header();
 
     mir::protobuf::wire::Result read_message_body(const size_t body_size);
+    void notify_disconnected();
 
     std::shared_ptr<SurfaceMap> surface_map;
     std::shared_ptr<DisplayConfiguration> display_configuration;