Merge lp:~albaguirre/mir/alternate-fix-1285215 into lp:mir

[Alberto Aguirre]

Remove stale socket files (LP: #1285215)

Stale socket files can be detected by checking the active socket connection table
at /proc/net/unix.
Stale socket files are files that exist, are of socket type
but don't have an active socket connection entry.

Remove stale socket files (LP: #1285215)

Stale socket files can be detected by checking the active socket connection table
at /proc/net/unix.
Stale socket files are files that exist, are of socket type
but don't have an active socket connection entry.

This is an alternative solution to switching to using the abstract socket namespace.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1547
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1307/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1570
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1568
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1143/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1039/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1044
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1044/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1144
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1144/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1059/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5940

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1307/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1549
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1309/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1572
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1570
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1145
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1041
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1041/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1046
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1046/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1146
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1146/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1061
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5944

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1309/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

60 + socket_path.insert(std::begin(socket_path), ' ');
69 + if (line.find(socket_path) != std::string::npos)

We also need to ensure that what we have found is a full word match, not a partial match. For example searching for '/tmp/mir_socket' shouldn't match an entry for '/tmp/mir_socket_host'.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> 60 + socket_path.insert(std::begin(socket_path), ' ');
> 69 + if (line.find(socket_path) != std::string::npos)
>
> We also need to ensure that what we have found is a full word match, not a
> partial match. For example searching for '/tmp/mir_socket' shouldn't match an
> entry for '/tmp/mir_socket_host'.

+1

(I'm still not convinced we should be doing this at all.)

review: Needs Fixing

[Robert Carr (robertcarr)]

I would feel better if we knew how the socket could get orphaned now? What sort of signals?

review: Needs Information

[Robert Carr (robertcarr)]

run_mir.cpp:
>> auto const intercepted = { SIGQUIT, SIGABRT, SIGFPE, SIGSEGV, SIGBUS };

SIGPIPE looks like the most likely culprit?

[Alberto Aguirre (albaguirre)]

> I would feel better if we knew how the socket could get orphaned now? What
> sort of signals?

SIGKILL for sure :)

[Alberto Aguirre (albaguirre)]

> > I would feel better if we knew how the socket could get orphaned now? What
> > sort of signals?
>
> SIGKILL for sure :)

For example what if you driver hangs and you need to really SIGKILL the server? Some would argue it is the responsibility of whoever sent SIGKILL to remove the stale socket - a fair point. However I - obviously since I submitted this MP - am of the opinion that if we can detect the stale socket then mir should be able to remove it.

[Alberto Aguirre (albaguirre)]

> We also need to ensure that what we have found is a full word match, not a
> partial match. For example searching for '/tmp/mir_socket' shouldn't match an
> entry for '/tmp/mir_socket_host'.

Fixed, although the only effect it had was a false positive detection of an active socket connection.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1550
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1322/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1590
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1588
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1163
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1054
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1054/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1059
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1059/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1164
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1164/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1080
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6021

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1322/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

This adds complexity that I don't think we need for a small convenience when the server is SIGKILLed. But the impact is localized and not worth arguing over.

review: Approve

[Robert Carr (robertcarr)]

review: Approve

[Alexandros Frantzis (afrantzis)]

OK.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/576/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1633
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1631
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1206/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/481
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-autolanding/481/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/482
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-autolanding/482/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1207
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1207/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1116/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6124

review: Needs Fixing (continuous-integration)

[Alberto Aguirre (albaguirre)]

^--Looks like a CI timeout to me...

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'include/test/mir_test_framework/display_server_test_fixture.h'
--- include/test/mir_test_framework/display_server_test_fixture.h	2014-03-06 06:05:17 +0000
+++ include/test/mir_test_framework/display_server_test_fixture.h	2014-04-14 23:26:11 +0000
@@ -65,6 +65,7 @@
 
     bool shutdown_server_process();
     Result  wait_for_shutdown_server_process();
+    bool kill_server_process();
     void kill_client_processes();
     void terminate_client_processes();
 

=== modified file 'include/test/mir_test_framework/testing_process_manager.h'
--- include/test/mir_test_framework/testing_process_manager.h	2014-03-06 06:05:17 +0000
+++ include/test/mir_test_framework/testing_process_manager.h	2014-04-14 23:26:11 +0000
@@ -58,6 +58,7 @@
     void tear_down_server();
     void tear_down_all();
     Result shutdown_server_process();
+    Result kill_server_process();
     Result wait_for_shutdown_server_process();
     void kill_client_processes();
     void terminate_client_processes();

=== modified file 'src/server/frontend/published_socket_connector.cpp'
--- src/server/frontend/published_socket_connector.cpp	2014-03-06 06:05:17 +0000
+++ src/server/frontend/published_socket_connector.cpp	2014-04-14 23:26:11 +0000
@@ -26,18 +26,82 @@
 #include <boost/throw_exception.hpp>
 
 #include <sys/socket.h>
+#include <sys/stat.h>
+
+#include <cstdio>
+#include <fstream>
 
 namespace mf = mir::frontend;
 namespace mfd = mir::frontend::detail;
 namespace ba = boost::asio;
 
+namespace
+{
+
+bool socket_file_exists(std::string const& filename)
+{
+    struct stat statbuf;
+    bool exists = (0 == stat(filename.c_str(), &statbuf));
+    /* Avoid removing non-socket files */
+    bool is_socket_type = (statbuf.st_mode & S_IFMT) == S_IFSOCK;
+    return exists && is_socket_type;
+}
+
+bool socket_exists(std::string const& socket_name)
+{
+    try
+    {
+        std::string socket_path{socket_name};
+
+        /* In case an abstract socket name exists with the same name*/
+        socket_path.insert(std::begin(socket_path), ' ');
+
+        /* If the name is contained in this table, it signifies
+         * a process is truly using that socket connection
+         */
+        std::ifstream socket_names_file("/proc/net/unix");
+        std::string line;
+        while (std::getline(socket_names_file, line))
+        {
+           auto index = line.find(socket_path);
+           /* check for complete match */
+           if (index != std::string::npos &&
+               (index + socket_path.length()) == line.length())
+           {
+               return true;
+           }
+        }
+    }
+    catch (...)
+    {
+        /* Assume the socket exists */
+        return true;
+    }
+    return false;
+}
+
+std::string remove_if_stale(std::string const& socket_name)
+{
+    if (socket_file_exists(socket_name) && !socket_exists(socket_name))
+    {
+        if (std::remove(socket_name.c_str()) != 0)
+        {
+            BOOST_THROW_EXCEPTION(
+                boost::enable_error_info(
+                    std::runtime_error("Failed removing stale socket file")) << boost::errinfo_errno(errno));
+        }
+    }
+    return socket_name;
+}
+}
+
 mf::PublishedSocketConnector::PublishedSocketConnector(
     const std::string& socket_file,
     std::shared_ptr<SessionCreator> const& session_creator,
     int threads,
     std::shared_ptr<ConnectorReport> const& report)
 :   BasicConnector(session_creator, threads, report),
-    socket_file(socket_file),
+    socket_file(remove_if_stale(socket_file)),
     acceptor(io_service, socket_file)
 {
     start_accept();

=== modified file 'tests/acceptance-tests/test_server_startup.cpp'
--- tests/acceptance-tests/test_server_startup.cpp	2013-01-11 23:47:40 +0000
+++ tests/acceptance-tests/test_server_startup.cpp	2014-04-14 23:26:11 +0000
@@ -49,4 +49,35 @@
 
     launch_client_process(client_config);
 }
+
+TEST_F(BespokeDisplayServerTestFixture, can_start_new_instance_after_sigkill)
+{
+    ASSERT_FALSE(mtf::detect_server(mtf::test_socket_file(), std::chrono::milliseconds(0)));
+
+    TestingServerConfiguration config;
+    launch_server_process(config);
+
+    run_in_test_process([&]
+    {
+        /* Under valgrind, raise(SIGKILL) results in a memcheck orphan process
+         * we kill the server process from the test process instead
+         */
+        EXPECT_TRUE(kill_server_process());
+
+        /* Attempt to start a new server instance */
+        TestingServerConfiguration server_config;
+        launch_server_process(server_config);
+
+        struct ClientConfig : TestingClientConfiguration
+        {
+            void exec()
+            {
+                EXPECT_TRUE(mtf::detect_server(mtf::test_socket_file(),
+                                               std::chrono::milliseconds(100)));
+            }
+        } client_config;
+
+        launch_client_process(client_config);
+    });
+}
 }

=== modified file 'tests/mir_test_framework/display_server_test_fixture.cpp'
--- tests/mir_test_framework/display_server_test_fixture.cpp	2014-03-06 06:05:17 +0000
+++ tests/mir_test_framework/display_server_test_fixture.cpp	2014-04-14 23:26:11 +0000
@@ -75,6 +75,13 @@
         || result.reason == TerminationReason::unknown;
 }
 
+bool BespokeDisplayServerTestFixture::kill_server_process()
+{
+    auto const& result = process_manager.kill_server_process();
+    return result.reason == TerminationReason::child_terminated_by_signal &&
+           result.signal == SIGKILL;
+}
+
 mtf::Result BespokeDisplayServerTestFixture::wait_for_shutdown_server_process()
 {
     return process_manager.wait_for_shutdown_server_process();

=== modified file 'tests/mir_test_framework/process.cpp'
--- tests/mir_test_framework/process.cpp	2014-03-06 06:05:17 +0000
+++ tests/mir_test_framework/process.cpp	2014-04-14 23:26:11 +0000
@@ -122,6 +122,11 @@
                     result.signal = WTERMSIG(status);
                     break;
                 }
+                else {
+                    terminated = true;
+                    result.reason = TerminationReason::unknown;
+                    break;
+                }
             }
             else if (rc == 0)
             {

=== modified file 'tests/mir_test_framework/testing_process_manager.cpp'
--- tests/mir_test_framework/testing_process_manager.cpp	2014-03-06 06:05:17 +0000
+++ tests/mir_test_framework/testing_process_manager.cpp	2014-04-14 23:26:11 +0000
@@ -165,6 +165,25 @@
     return result;
 }
 
+mtf::Result mtf::TestingProcessManager::kill_server_process()
+{
+    Result result;
+
+    if (server_process)
+    {
+        server_process->kill();
+        result = server_process->wait_for_termination();
+        server_process.reset();
+        return result;
+    }
+    else
+    {
+        result.reason = TerminationReason::unknown;
+    }
+
+    return result;
+}
+
 mtf::Result mtf::TestingProcessManager::wait_for_shutdown_server_process()
 {
     Result result;

=== modified file 'tests/mir_test_framework/testing_server_options.cpp'
--- tests/mir_test_framework/testing_server_options.cpp	2014-03-06 06:05:17 +0000
+++ tests/mir_test_framework/testing_server_options.cpp	2014-04-14 23:26:11 +0000
@@ -23,8 +23,50 @@
 #include <boost/exception/errinfo_errno.hpp>
 #include <boost/throw_exception.hpp>
 
+#include <random>
+#include <fstream>
+
 namespace mtf = mir_test_framework;
 
+namespace
+{
+
+bool socket_exists(std::string const& socket_name)
+{
+    std::string socket_path{socket_name};
+    socket_path.insert(std::begin(socket_path), ' ');
+
+    std::ifstream socket_names_file("/proc/net/unix");
+    std::string line;
+    while (std::getline(socket_names_file, line))
+    {
+       if (line.find(socket_path) != std::string::npos)
+           return true;
+    }
+    return false;
+}
+
+std::string create_random_socket_name()
+{
+    std::random_device random_device;
+    std::mt19937 generator(random_device());
+    int max_concurrent_test_instances = 99999;
+    std::uniform_int_distribution<> dist(1, max_concurrent_test_instances);
+    std::string const suffix{"-mir_socket_test"};
+
+    /* check for name collisions against other test instances
+     * running concurrently
+     */
+    for (int i = 0; i < max_concurrent_test_instances; i++)
+    {
+        std::string name{std::to_string(dist(generator))};
+        name.append(suffix);
+        if (!socket_exists(name))
+            return name;
+    }
+    throw std::runtime_error("Too many test socket instances exist!");
+}
+}
 
 mtf::TestingServerConfiguration::TestingServerConfiguration() :
     using_server_started_sync(false)
@@ -99,6 +141,6 @@
 
 std::string const& mtf::test_socket_file()
 {
-    static const std::string socket_file{"./mir_socket_test"};
+    static const std::string socket_file{create_random_socket_name()};
     return socket_file;
 }