Merge lp:~afrantzis/mir/fix-1201436-more into lp:mir

[Alexandros Frantzis]

client: Allow clients to call API functions after a connection break has been detected

When a client tries to call an API function after a connection break has
been detected in a previous API call, the client blocks in the new call.
This happens because in MirSocketRpcChannel::notify_disconnected() the
pending RPC calls are not forced to complete, since the channel has already
been marked as 'disconnected' by the failure in the previous call.

Note that if the break is first detected while calling an API function,
then that call doesn't block, since this is the first time we call
MirSocketRpcChannel::notify_disconnected() and the pending RPC calls are
forced to complete.

This commit solves this problem by always forcing requests to complete
when a communication failure occurs, even if a disconnection has
already been handled. This is preferred over the alternative of
manually calling the completion callback in a try-catch block when
calling an RPC method because of:

  1. Correctness: In case the communication problem first occurs in that
     call, the callback will be called twice, once by notify_disconnected()
     and once manually.

  2. Consistency: The callback is called from one place regardless of
     whether the communication problem is first detected during that
     call or not.

client: Allow clients to call API functions after a connection break has been detected

When a client tries to call an API function after a connection break has
been detected in a previous API call, the client blocks in the new call.
This happens because in MirSocketRpcChannel::notify_disconnected() the
pending RPC calls are not forced to complete, since the channel has already
been marked as 'disconnected' by the failure in the previous call.

Note that if the break is first detected while calling an API function,
then that call doesn't block, since this is the first time we call
MirSocketRpcChannel::notify_disconnected() and the pending RPC calls are
forced to complete.

This commit solves this problem by always forcing requests to complete
when a communication failure occurs, even if a disconnection has
already been handled. This is preferred over the alternative of
manually calling the completion callback in a try-catch block when
calling an RPC method because of:

  1. Correctness: In case the communication problem first occurs in that
     call, the callback will be called twice, once by notify_disconnected()
     and once manually.

  2. Consistency: The callback is called from one place regardless of
     whether the communication problem is first detected during that
     call or not.

I took this opportunity to do some other (somewhat related) cleanup in the client API implemenation.

[PS Jenkins bot (ps-jenkins)]

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/195/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1150
http://jenkins.qa.ubuntu.com/job/mir-ci/1596/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2416
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2301
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/840
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/840/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/97
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/97/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-ci/1596/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

16 {
17 - return mir_connect_impl(socket_file, name, callback, context);
18 + try
19 + {
20 + return mir_connect_impl(socket_file, name, callback, context);
21 + }
22 + catch (std::exception const&)
23 + {
24 + return nullptr;
25 + }
26 }

Two things - this could be a function try block and save a couple of lines. And, surely mir_connect_impl() should not be throwing.

28 void mir_connection_release(MirConnection *connection)
29 {
30 - return mir_connection_release_impl(connection);
31 + try
32 + {
33 + return mir_connection_release_impl(connection);
34 + }
35 + catch (std::exception const&)
36 + {
37 + }
38 }

Ditto

[Alan Griffiths (alan-griffiths)]

128 + surf->configure(mir_surface_attrib_state,
129 + mir_surface_state_unknown)->wait_for_all();

I guess it is pre-existing - but this is a blocking call in a function that isn't flagged "_sync".

[Alan Griffiths (alan-griffiths)]

There's more work to do around here, but this doesn't create any new problems

review: Approve

[Daniel van Vugt (vanvugt)]

I suspect this conflicts with potential resolutions of bug 1239978, but don't have a strong opinion (yet).

review: Abstain

[Robert Carr (robertcarr)]

Good to unravel this. One of these days the stress test is going to pass...(I thought this might be related to the hanging client issue which remains there, but can't rationalize any path).

I do feel like we should be validating arguments better, but probably to return error codes, not to crash/throw as in 1239978. Let's save this as a topic for later though.

review: Approve

[Daniel van Vugt (vanvugt)]

I don't think it's a good idea to allow buggy clients to keep executing and silently skipping subsequent operations. Then Mir gets blamed for bugs in clients and we waste precious time tracking them down. See bug 1239978.

[Daniel van Vugt (vanvugt)]

Although, losing a connection for reasons beyond the client's control is not a bug in the client. I agree we should unblock it in that case. And now I look again, I don't think this is fundamentally incompatible with bug 1239978.

I'd only note that this being a C API, we should return a C error value (NULL) rather than nullptr.

review: Abstain

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-10-17 05:51:24 +0000
+++ src/client/mir_client_library.cpp	2013-10-18 11:29:15 +0000
@@ -105,7 +105,7 @@
         error_connections.insert(error_connection);
         error_connection->set_error_message(x.what());
         callback(error_connection, context);
-        return 0;
+        return nullptr;
     }
 }
 
@@ -137,12 +137,25 @@
 
 MirWaitHandle* mir_connect(char const* socket_file, char const* name, mir_connected_callback callback, void * context)
 {
-    return mir_connect_impl(socket_file, name, callback, context);
+    try
+    {
+        return mir_connect_impl(socket_file, name, callback, context);
+    }
+    catch (std::exception const&)
+    {
+        return nullptr;
+    }
 }
 
 void mir_connection_release(MirConnection *connection)
 {
-    return mir_connection_release_impl(connection);
+    try
+    {
+        return mir_connection_release_impl(connection);
+    }
+    catch (std::exception const&)
+    {
+    }
 }
 
 MirConnection *mir_connect_sync(char const *server,
@@ -194,7 +207,7 @@
     catch (std::exception const&)
     {
         // TODO callback with an error surface
-        return 0; // TODO
+        return nullptr;
     }
 
 }
@@ -222,7 +235,14 @@
     MirSurface * surface,
     mir_surface_callback callback, void * context)
 {
-    return surface->release_surface(callback, context);
+    try
+    {
+        return surface->release_surface(callback, context);
+    }
+    catch (std::exception const&)
+    {
+        return nullptr;
+    }
 }
 
 void mir_surface_release_sync(MirSurface *surface)
@@ -351,7 +371,7 @@
 }
 catch (std::exception const&)
 {
-    return 0;
+    return nullptr;
 }
 
 void mir_surface_swap_buffers_sync(MirSurface *surface)
@@ -387,9 +407,16 @@
 }
 
 MirWaitHandle* mir_surface_set_type(MirSurface *surf,
-                                                           MirSurfaceType type)
+                                    MirSurfaceType type)
 {
-    return surf ? surf->configure(mir_surface_attrib_type, type) : NULL;
+    try
+    {
+        return surf ? surf->configure(mir_surface_attrib_type, type) : nullptr;
+    }
+    catch (std::exception const&)
+    {
+        return nullptr;
+    }
 }
 
 MirSurfaceType mir_surface_get_type(MirSurface *surf)
@@ -410,25 +437,38 @@
 
 MirWaitHandle* mir_surface_set_state(MirSurface *surf, MirSurfaceState state)
 {
-    return surf ? surf->configure(mir_surface_attrib_state, state) : NULL;
+    try
+    {
+        return surf ? surf->configure(mir_surface_attrib_state, state) : nullptr;
+    }
+    catch (std::exception const&)
+    {
+        return nullptr;
+    }
 }
 
 MirSurfaceState mir_surface_get_state(MirSurface *surf)
 {
     MirSurfaceState state = mir_surface_state_unknown;
 
-    if (surf)
+    try
     {
-        int s = surf->attrib(mir_surface_attrib_state);
-
-        if (s == mir_surface_state_unknown)
+        if (surf)
         {
-            surf->configure(mir_surface_attrib_state,
-                            mir_surface_state_unknown)->wait_for_all();
-            s = surf->attrib(mir_surface_attrib_state);
+            int s = surf->attrib(mir_surface_attrib_state);
+
+            if (s == mir_surface_state_unknown)
+            {
+                surf->configure(mir_surface_attrib_state,
+                                mir_surface_state_unknown)->wait_for_all();
+                s = surf->attrib(mir_surface_attrib_state);
+            }
+
+            state = static_cast<MirSurfaceState>(s);
         }
-
-        state = static_cast<MirSurfaceState>(s);
+    }
+    catch (std::exception const&)
+    {
     }
 
     return state;
@@ -437,8 +477,16 @@
 MirWaitHandle* mir_surface_set_swapinterval(MirSurface* surf, int interval)
 {
     if ((interval < 0) || (interval > 1))
-        return NULL;
-    return surf ? surf->configure(mir_surface_attrib_swapinterval, interval) : NULL;
+        return nullptr;
+
+    try
+    {
+        return surf ? surf->configure(mir_surface_attrib_swapinterval, interval) : nullptr;
+    }
+    catch (std::exception const&)
+    {
+        return nullptr;
+    }
 }
 
 int mir_surface_get_swapinterval(MirSurface* surf)
@@ -462,8 +510,12 @@
 
 MirWaitHandle* mir_connection_apply_display_config(MirConnection *connection, MirDisplayConfiguration* display_configuration)
 {
-    if (!connection)
-        return NULL;
- 
-    return connection->configure_display(display_configuration);
+    try
+    {
+        return connection ? connection->configure_display(display_configuration) : nullptr;
+    }
+    catch (std::exception const&)
+    {
+        return nullptr;
+    }
 }

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2013-10-15 08:53:10 +0000
+++ src/client/mir_connection.cpp	2013-10-18 11:29:15 +0000
@@ -185,21 +185,17 @@
 
     SurfaceRelease surf_release{surface, new_wait_handle, callback, context};
 
-    try
-    {
-        mir::protobuf::SurfaceId message;
-        message.set_value(surface->id());
-        server.release_surface(0, &message, &void_response,
-                        gp::NewCallback(this, &MirConnection::released, surf_release));
-    }
-    catch (std::exception const& x)
-    {
-        set_error_message(std::string("release_surface: ") + x.what());
-        released(surf_release);
-    }
-
-    std::lock_guard<std::mutex> rel_lock(release_wait_handle_guard);
-    release_wait_handles.push_back(new_wait_handle);
+    mir::protobuf::SurfaceId message;
+    message.set_value(surface->id());
+
+    {
+        std::lock_guard<std::mutex> rel_lock(release_wait_handle_guard);
+        release_wait_handles.push_back(new_wait_handle);
+    }
+
+    server.release_surface(0, &message, &void_response,
+                           gp::NewCallback(this, &MirConnection::released, surf_release));
+
 
     return new_wait_handle;
 }
@@ -282,15 +278,8 @@
 {
     std::lock_guard<std::recursive_mutex> lock(mutex);
 
-    try 
-    {
-        server.disconnect(0, &ignored, &ignored,
-                          google::protobuf::NewCallback(this, &MirConnection::done_disconnect));
-    }
-    catch (std::exception const& x)
-    {
-        set_error_message(std::string("disconnect: ") + x.what());
-    }
+    server.disconnect(0, &ignored, &ignored,
+                      google::protobuf::NewCallback(this, &MirConnection::done_disconnect));
 
     return &disconnect_wait_handle;
 }

=== modified file 'src/client/rpc/mir_socket_rpc_channel.cpp'
--- src/client/rpc/mir_socket_rpc_channel.cpp	2013-10-17 05:51:24 +0000
+++ src/client/rpc/mir_socket_rpc_channel.cpp	2013-10-18 11:29:15 +0000
@@ -92,8 +92,8 @@
         io_service.stop();
         socket.close();
         lifecycle_control->call_lifecycle_event_handler(mir_lifecycle_connection_lost);
-        pending_calls.force_completion();
     }
+    pending_calls.force_completion();
 }
 
 void mclr::MirSocketRpcChannel::init()

=== modified file 'tests/acceptance-tests/test_server_disconnect.cpp'
--- tests/acceptance-tests/test_server_disconnect.cpp	2013-10-04 10:51:37 +0000
+++ tests/acceptance-tests/test_server_disconnect.cpp	2013-10-18 11:29:15 +0000
@@ -20,6 +20,7 @@
 
 #include "mir_test_framework/display_server_test_fixture.h"
 #include "mir_test_framework/cross_process_sync.h"
+#include "mir_test/cross_process_action.h"
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
@@ -27,6 +28,7 @@
 #include <atomic>
 
 namespace mtf = mir_test_framework;
+namespace mt = mir::test;
 
 using ServerDisconnect = mtf::BespokeDisplayServerTestFixture;
 
@@ -75,6 +77,57 @@
 
     mtf::CrossProcessSync sync;
 };
+
+struct DisconnectingTestingClientConfiguration : mtf::TestingClientConfiguration
+{
+    void exec() override
+    {
+        MirConnection* connection{nullptr};
+
+        connect.exec([&] {
+            connection = mir_connect_sync(mtf::test_socket_file().c_str() , __PRETTY_FUNCTION__);
+            EXPECT_TRUE(mir_connection_is_valid(connection));
+            /*
+             * Set a null callback to avoid killing the process
+             * (default callback raises SIGTERM).
+             */
+            mir_connection_set_lifecycle_event_callback(connection,
+                                                        null_lifecycle_callback,
+                                                        nullptr);
+        });
+
+        create_surface.exec([&] {
+            MirSurfaceParameters const parameters =
+            {
+                __PRETTY_FUNCTION__,
+                1, 1,
+                mir_pixel_format_abgr_8888,
+                mir_buffer_usage_hardware,
+                mir_display_output_id_invalid
+            };
+            mir_connection_create_surface_sync(connection, &parameters);
+        });
+
+        configure_display.exec([&] {
+            auto config = mir_connection_create_display_config(connection);
+            mir_wait_for(mir_connection_apply_display_config(connection, config));
+            mir_display_config_destroy(config);
+        });
+
+        disconnect.exec([&] {
+            mir_connection_release(connection);
+        });
+    }
+
+    static void null_lifecycle_callback(MirConnection*, MirLifecycleState, void*)
+    {
+    }
+
+    mt::CrossProcessAction connect;
+    mt::CrossProcessAction create_surface;
+    mt::CrossProcessAction configure_display;
+    mt::CrossProcessAction disconnect;
+};
 }
 
 TEST_F(ServerDisconnect, client_detects_server_shutdown)
@@ -91,3 +144,25 @@
         shutdown_server_process();
     });
 }
+
+TEST_F(ServerDisconnect, client_can_call_connection_functions_after_connection_break_is_detected)
+{
+    TestingServerConfiguration server_config;
+    launch_server_process(server_config);
+
+    DisconnectingTestingClientConfiguration client_config;
+    launch_client_process(client_config);
+
+    run_in_test_process([this, &client_config]
+    {
+        client_config.connect();
+        shutdown_server_process();
+        /* While trying to create a surface the connection break will be detected */
+        client_config.create_surface();
+
+        /* Trying to configure the display shouldn't block */
+        client_config.configure_display();
+        /* Trying to disconnect at this point shouldn't block */
+        client_config.disconnect();
+    });
+}