Merge lp:~afrantzis/mir/fix-1517781-double-close into lp:mir

[Alexandros Frantzis]

tests: Avoid double-closing the mir connection fd

tests: Avoid double-closing the mir connection fd

Besides being obviously wrong, it can lead to very mysterious failures in completely unrelated parts of the code (see bug 1517781).

I wonder if this is the cause of any other mysterious failures we have seen.

[Alan Griffiths (alan-griffiths)]

Makes sense.

But I can still reproduce problems in e.g. NestedServer.display_configuration_reset_when_application_exits

Vis:

Repeating all tests (iteration 429) . . .

Note: Google Test filter = NestedServer.display_configuration_reset_when_application_exits
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from NestedServer
[ RUN ] NestedServer.display_configuration_reset_when_application_exits
/home/alan/display_server/mir3/tests/acceptance-tests/test_nested_mir.cpp:755: Failure
Value of: condition.woken()
  Actual: false
Expected: true
Segmentation fault (core dumped)

So which NestedServer test failures are covered by the linked bug?

review: Approve

[Alexandros Frantzis (afrantzis)]

> So which NestedServer test failures are covered by the linked bug?

That's a different bug. This fix covers only the cases listed in the linked bug's description (hangs and exceptions involving bad file descriptors).

[Alberto Aguirre (albaguirre)]

OK.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3109
http://jenkins.qa.ubuntu.com/job/mir-ci/5599/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4937
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3843
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4883
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/839/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1752
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1752/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/837
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/837/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4883
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4883/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7450
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25293
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/840/console

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

review: Approve (continuous-integration)

=== modified file 'include/test/mir_test_framework/async_server_runner.h'
--- include/test/mir_test_framework/async_server_runner.h	2015-07-27 07:24:50 +0000
+++ include/test/mir_test_framework/async_server_runner.h	2015-11-19 16:26:10 +0000
@@ -52,7 +52,7 @@
     auto new_connection() -> std::string;
 
     /// \return a connection string for a client to connect to the server
-    auto connection(mir::Fd fd) -> std::string;
+    auto connection(int fd) -> std::string;
 
     mir::Server server;
 
@@ -60,8 +60,6 @@
     std::list<TemporaryEnvironmentValue> env;
     mir::test::AutoJoinThread server_thread;
 
-    std::list<mir::Fd> connections;
-
     std::mutex mutex;
     std::condition_variable started;
     bool server_running{false};

=== modified file 'tests/mir_test_framework/async_server_runner.cpp'
--- tests/mir_test_framework/async_server_runner.cpp	2015-07-27 07:24:50 +0000
+++ tests/mir_test_framework/async_server_runner.cpp	2015-11-19 16:26:10 +0000
@@ -110,7 +110,6 @@
 {
     server.stop();
     wait_for_server_exit();
-    connections.clear();
 }
 
 void mtf::AsyncServerRunner::wait_for_server_exit()
@@ -132,10 +131,12 @@
     return connection(server.open_client_socket());
 }
 
-auto mtf::AsyncServerRunner::connection(mir::Fd fd) -> std::string
+auto mtf::AsyncServerRunner::connection(int fd) -> std::string
 {
     char connect_string[64] = {0};
-    connections.push_back(fd);
-    sprintf(connect_string, "fd://%d", fd.operator int());
+    // We can't have both the server and the client owning the same fd, since
+    // that will result in a double-close(). We give the client a duplicate which
+    // the client can safely own (and should close when done).
+    sprintf(connect_string, "fd://%d", dup(fd));
     return connect_string;
 }