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

[Alan Griffiths]

client, tests: deal correctly with a race condition during double
disconnection test (LP: #1236698)

client, tests: deal correctly with a race condition during double disconnection test

[Alan Griffiths (alan-griffiths)]

Sorry, the SetUpTestCase, SetUp and TearDown changes are not essential to the fix (but are useful tidy-up).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1126
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/147/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2319
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2204
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/144
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/144/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/147/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Seems to do the trick.

review: Approve

[Daniel van Vugt (vanvugt)]

I'm going to rush this one through, because the spurious CI failures are holding up other MPs.

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2013-10-07 09:27:32 +0000
+++ src/client/mir_connection.cpp	2013-10-10 15:41:01 +0000
@@ -288,7 +288,6 @@
     catch (std::exception const& x)
     {
         set_error_message(std::string("disconnect: ") + x.what());
-        disconnect_wait_handle.result_received();
     }
 
     return &disconnect_wait_handle;
 
=== modified file 'tests/unit-tests/frontend/test_published_socket_connector.cpp'
--- tests/unit-tests/frontend/test_published_socket_connector.cpp	2013-10-07 09:27:32 +0000
+++ tests/unit-tests/frontend/test_published_socket_connector.cpp	2013-10-10 15:41:01 +0000
@@ -49,6 +49,18 @@
 class MockConnectorReport : public mf::ConnectorReport
 {
 public:
+    MockConnectorReport()
+    {
+        using namespace testing;
+        EXPECT_CALL(*this, thread_start()).Times(AnyNumber());
+        EXPECT_CALL(*this, thread_end()).Times(AnyNumber());
+        EXPECT_CALL(*this, starting_threads(_)).Times(AnyNumber());
+        EXPECT_CALL(*this, stopping_threads(_)).Times(AnyNumber());
+        EXPECT_CALL(*this, creating_session_for(_)).Times(AnyNumber());
+        EXPECT_CALL(*this, creating_socket_pair(_, _)).Times(AnyNumber());
+        EXPECT_CALL(*this, listening_on(_)).Times(AnyNumber());
+        EXPECT_CALL(*this, error(_)).Times(AnyNumber());
+    }
 
     ~MockConnectorReport() noexcept {}
 
@@ -68,8 +80,10 @@
 
 struct PublishedSocketConnector : public ::testing::Test
 {
+    static const char* const test_socket;
     static void SetUpTestCase()
     {
+        remove(test_socket);
     }
 
     void SetUp()
@@ -77,23 +91,20 @@
         communicator_report = std::make_shared<MockConnectorReport>();
         stub_server_tool = std::make_shared<mt::StubServerTool>();
         stub_server = std::make_shared<mt::TestProtobufServer>(
-            "./test_socket",
+            test_socket,
             stub_server_tool,
             communicator_report);
 
-        using namespace testing;
-        EXPECT_CALL(*communicator_report, error(_)).Times(AnyNumber());
         stub_server->comm->start();
-        client = std::make_shared<mt::TestProtobufClient>("./test_socket", 100);
+        client = std::make_shared<mt::TestProtobufClient>(test_socket, 100);
         client->connect_parameters.set_application_name(__PRETTY_FUNCTION__);
     }
 
     void TearDown()
     {
+        client.reset();
+
         stub_server->comm->stop();
-        testing::Mock::VerifyAndClearExpectations(communicator_report.get());
-        client.reset();
-
         stub_server.reset();
         stub_server_tool.reset();
         communicator_report.reset();
@@ -109,6 +120,7 @@
     static std::shared_ptr<mt::TestProtobufServer> stub_server;
 };
 
+const char* const PublishedSocketConnector::test_socket = "./test_socket";
 std::shared_ptr<mt::StubServerTool> PublishedSocketConnector::stub_server_tool;
 std::shared_ptr<MockConnectorReport> PublishedSocketConnector::communicator_report;
 std::shared_ptr<mt::TestProtobufServer> PublishedSocketConnector::stub_server;
@@ -196,7 +208,7 @@
 
 }
 
-TEST_F(PublishedSocketConnector, double_disconnection_attempt_throws_exception)
+TEST_F(PublishedSocketConnector, double_disconnection_does_not_break)
 {
     using namespace testing;
 
@@ -220,13 +232,33 @@
 
     Mock::VerifyAndClearExpectations(client.get());
 
-    EXPECT_CALL(*client->rpc_report, invocation_failed(InvocationMethodEq("disconnect"),_));
+    // There is a race between the server closing the socket and
+    // the client sending the second "disconnect".
+    // Usually[*] the server wins and the "disconnect" fails resulting
+    // in an invocation_failed() report and an exception.
+    // Occasionally, the client wins and the message is sent to dying socket.
+    // [*] ON my desktop under valgrind "Usually" is ~49 times out of 50
+
+    EXPECT_CALL(*client->rpc_report, invocation_failed(InvocationMethodEq("disconnect"),_)).Times(AtMost(1));
     EXPECT_CALL(*client, disconnect_done()).Times(1);
 
-    EXPECT_THROW({
-    client->display_server.disconnect(0, &client->ignored, &client->ignored, google::protobuf::NewCallback(client.get(), &mt::TestProtobufClient::disconnect_done));
+    try
+    {
+        client->display_server.disconnect(
+            0,
+            &client->ignored,
+            &client->ignored,
+            google::protobuf::NewCallback(client.get(), &mt::TestProtobufClient::disconnect_done));
+
+        // the write beat the socket closing
+    }
+    catch (std::runtime_error const& x)
+    {
+        // the socket closing beat the write
+        EXPECT_THAT(x.what(), HasSubstr("Failed to send message to server:"));
+    }
+
     client->wait_for_disconnect_done();
-    }, std::runtime_error);
 }
 
 TEST_F(PublishedSocketConnector, getting_and_advancing_buffers)