Merge lp:~mir-team/mir/fix-1245336.2 into lp:mir

[Daniel van Vugt]

Avoid a race condition that could lead to spurious failures of server
shutdown tests (LP: #1245336)

The race is between the acceptance-tests process and its child server process.
In our signal/abort tests, it's possible the server process may start and
stop so quickly that the parent process doesn't have time to detect the
socket file (before it's correctly cleaned up).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1173
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/281/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/37
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/36
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/10
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/10/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/10
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/10/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/281/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1174
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/284/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/40
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/39
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/13
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/13/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/13
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/13/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/284/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

This seems overly complicated - c.f. https://code.launchpad.net/~alan-griffiths/mir/fix-1245336/+merge/193093

review: Disapprove

=== modified file 'include/test/mir_test_framework/display_server_test_fixture.h'
--- include/test/mir_test_framework/display_server_test_fixture.h	2013-09-26 15:07:59 +0000
+++ include/test/mir_test_framework/display_server_test_fixture.h	2013-10-29 11:05:03 +0000
@@ -67,6 +67,9 @@
     void kill_client_processes();
     void terminate_client_processes();
 
+    bool wait_for_server_to_pause();
+    void resume_server_process();
+
     void run_in_test_process(std::function<void()> const& run_code);
 
 protected:

=== modified file 'include/test/mir_test_framework/process.h'
--- include/test/mir_test_framework/process.h	2013-08-28 03:41:48 +0000
+++ include/test/mir_test_framework/process.h	2013-10-29 11:05:03 +0000
@@ -73,6 +73,8 @@
     // Wait for the process to terminate, and return the results.
     Result wait_for_termination(const std::chrono::milliseconds& timeout = std::chrono::milliseconds(60 * 1000));
 
+    bool wait_for_stop(const std::chrono::milliseconds& timeout = std::chrono::milliseconds(60 * 1000));
+
     void kill();
     void terminate();
     void stop();

=== modified file 'include/test/mir_test_framework/testing_process_manager.h'
--- include/test/mir_test_framework/testing_process_manager.h	2013-09-25 18:25:11 +0000
+++ include/test/mir_test_framework/testing_process_manager.h	2013-10-29 11:05:03 +0000
@@ -60,6 +60,8 @@
     Result shutdown_server_process();
     void kill_client_processes();
     void terminate_client_processes();
+    bool wait_for_server_to_pause();
+    void resume_server();
     void run_in_test_process(std::function<void()> const& run_code);
 
 private:

=== modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
--- tests/acceptance-tests/test_server_shutdown.cpp	2013-10-25 09:01:53 +0000
+++ tests/acceptance-tests/test_server_shutdown.cpp	2013-10-29 11:05:03 +0000
@@ -375,19 +375,20 @@
     {
         void exec() override
         {
+            raise(SIGSTOP);
             abort();
         }
     };
 
     ServerConfig server_config;
     launch_server_process(server_config);
+    // ^ This blocks until the socket file exists
 
     run_in_test_process([&]
     {
-        using std::chrono::steady_clock;
-        std::chrono::seconds const limit(2);
-
+        ASSERT_TRUE(wait_for_server_to_pause());
         ASSERT_TRUE(file_exists(server_config.the_socket_file()));
+        resume_server_process();
 
         // shutdown should fail as the server aborted
         ASSERT_FALSE(shutdown_server_process());
@@ -404,6 +405,7 @@
     {
         void exec() override
         {
+            raise(SIGSTOP);
             raise(sig);
         }
 
@@ -413,13 +415,13 @@
 
     ServerConfig server_config(GetParam());
     launch_server_process(server_config);
+    // ^ This blocks until the socket file exists
 
     run_in_test_process([&]
     {
-        using std::chrono::steady_clock;
-        std::chrono::seconds const limit(2);
-
+        ASSERT_TRUE(wait_for_server_to_pause());
         ASSERT_TRUE(file_exists(server_config.the_socket_file()));
+        resume_server_process();
 
         // shutdown should fail as the server faulted
         ASSERT_FALSE(shutdown_server_process());

=== modified file 'tests/mir_test_framework/display_server_test_fixture.cpp'
--- tests/mir_test_framework/display_server_test_fixture.cpp	2013-09-26 15:07:59 +0000
+++ tests/mir_test_framework/display_server_test_fixture.cpp	2013-10-29 11:05:03 +0000
@@ -86,6 +86,16 @@
     process_manager.kill_client_processes();
 }
 
+bool BespokeDisplayServerTestFixture::wait_for_server_to_pause()
+{
+    return process_manager.wait_for_server_to_pause();
+}
+
+void BespokeDisplayServerTestFixture::resume_server_process()
+{
+    process_manager.resume_server();
+}
+
 void BespokeDisplayServerTestFixture::run_in_test_process(
     std::function<void()> const& run_code)
 {

=== modified file 'tests/mir_test_framework/process.cpp'
--- tests/mir_test_framework/process.cpp	2013-08-28 03:41:48 +0000
+++ tests/mir_test_framework/process.cpp	2013-10-29 11:05:03 +0000
@@ -166,6 +166,39 @@
     return result;
 }
 
+bool mtf::Process::wait_for_stop(const std::chrono::milliseconds& timeout)
+{
+    auto deadline = std::chrono::system_clock::now() + timeout;
+    bool stopped = false;
+
+    while (std::chrono::system_clock::now() < deadline)
+    {
+        int status;
+        int ret = waitpid(pid, &status, WNOHANG | WUNTRACED);
+
+        if (ret == pid)
+        {
+            if (WIFSTOPPED(status))
+            {
+                stopped = true;
+                break;
+            }
+            else if (WIFSIGNALED(status) || WIFEXITED(status))
+            {
+                break;
+            }
+        }
+        else if (ret < 0)
+        {
+            break;
+        }
+
+        std::this_thread::yield();
+    }
+
+    return stopped;
+}
+
 void mtf::Process::kill()
 {
     if (!detached) signal_process(pid, SIGKILL);

=== modified file 'tests/mir_test_framework/testing_process_manager.cpp'
--- tests/mir_test_framework/testing_process_manager.cpp	2013-09-30 09:18:00 +0000
+++ tests/mir_test_framework/testing_process_manager.cpp	2013-10-29 11:05:03 +0000
@@ -225,6 +225,24 @@
     }
 }
 
+bool mtf::TestingProcessManager::wait_for_server_to_pause()
+{
+    bool paused = false;
+
+    if (server_process)
+    {
+        paused = server_process->wait_for_stop(std::chrono::minutes(1));
+    }
+
+    return paused;
+}
+
+void mtf::TestingProcessManager::resume_server()
+{
+    if (server_process)
+        server_process->cont();
+}
+
 void mtf::TestingProcessManager::run_in_test_process(std::function<void()> const& run_code)
 {
     if (is_test_process)