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

[Alan Griffiths]

client: avoid lifecycle notifications racing with connection release.

client: avoid lifecycle notifications racing with connection release.

Not only our code, but also user code, is likely to assume lifecycle notifications can't happen once the connection has been released.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2008
http://jenkins.qa.ubuntu.com/job/mir-ci/1930/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2279
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2286
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2214
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/270/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1151
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1151/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3191
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15137

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> [ RUN ] DemoPrivateProtobuf.client_calls_server
> ==8885==
> ==8885== HEAP SUMMARY:
> ==8885== in use at exit: 211,741 bytes in 3,421 blocks
> ==8885== total heap usage: 9,029 allocs, 5,608 frees, 598,608 bytes allocated
> ==8885==
> ==8885== LEAK SUMMARY:
> ==8885== definitely lost: 0 bytes in 0 blocks
> ==8885== indirectly lost: 0 bytes in 0 blocks
> ==8885== possibly lost: 52,192 bytes in 940 blocks
> ==8885== still reachable: 159,549 bytes in 2,481 blocks
> ==8885== suppressed: 0 bytes in 0 blocks
> ==8885== Reachable blocks (those to which a pointer was found) are not shown.
> ==8885== To see them, rerun with: --leak-check=full --show-leak-kinds=all
> ==8885==
> ==8885== For counts of detected and suppressed errors, rerun with: -v
> ==8885== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
>
> test 11

:(

It appears the race is unrelated to this test failure.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2009
http://jenkins.qa.ubuntu.com/job/mir-ci/1931/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2280/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2287
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2215
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/271
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/271/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1152
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1152/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3193
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15147

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

A different error on a different architecture:

    E: Couldn't download packages: libisl10 libpthread-stubs0-dev sysv-rc

I guess an infrastructure glitch - triggering rebuild. (Which incidentally re-tries the AMD64 arch)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2009
http://jenkins.qa.ubuntu.com/job/mir-ci/1932/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2282
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2289
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/2217
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/272
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/272/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1154
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/1154/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3194
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/15152

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

LGTM.

review: Approve

[Kevin DuBois (kdub)]

makes sense to me

review: Approve

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2014-10-28 00:29:39 +0000
+++ src/client/mir_connection.cpp	2014-10-28 15:08:15 +0000
@@ -300,6 +300,8 @@
             delete handle;
     }
 
+    // Ensure no racy lifecycle notifications can happen after disconnect completes
+    lifecycle_control->set_lifecycle_event_handler([](MirLifecycleState){});
     disconnect_wait_handle.result_received();
 }
 

=== modified file 'tests/integration-tests/test_protobuf.cpp'
--- tests/integration-tests/test_protobuf.cpp	2014-10-27 17:30:19 +0000
+++ tests/integration-tests/test_protobuf.cpp	2014-10-28 15:08:15 +0000
@@ -205,6 +205,16 @@
     result.set_error(nothing_returned);
     std::atomic<bool> called_back{false};
 
+    // After the call there's a race between the client releasing the connection
+    // and the server dropping the connection.
+    // If the latter wins we'll invoke the client's lifecycle_event_callback.
+    // As the default callback kills the process with SIGHUP, we need to
+    // replace it to ensure the test can continue.
+    mir_connection_set_lifecycle_event_callback(
+        connection,
+        [](MirConnection*, MirLifecycleState, void*){},
+        nullptr);
+
     // Note:
     // As the default server won't recognise this call it drops the connection
     // resulting in a callback when the connection drops (but result being unchanged)
@@ -214,11 +224,6 @@
         &result,
         NewCallback(&callback, &called_back));
 
-    // FIXME - This test is somehow racy. If I add:
-    //    EXPECT_TRUE(false) << connection;
-    // then I can get mir_connection_release to generate an exception during
-    // disconnect() internally, sometimes. Although that exception is caught
-    // internally by the client library so we don't see it here.
     mir_connection_release(connection);
 
     EXPECT_TRUE(called_back);