Merge lp:~alan-griffiths/mir/remove-endpoint-first-when-shutting-down into lp:mir

[Alan Griffiths]

frontend: Remove the endpoint first when shutting down

frontend: Remove the endpoint first when shutting down

Ensures that the endpoint is removed from the filesystem at the start of the shutdown process. (So that, if there are problems later we are not left with a stale endpoint.)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1109
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/91/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2237
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2122
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/90
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/90/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

For a moment I thought this was a fix for bug 1235159. Now I realize it's not really. The main case (sadly) we need to deal with on that one is a stale socket file left by a crashing server.

[Daniel van Vugt (vanvugt)]

Hmm, actually, is this a good idea at all?

The proposal here only solves a minority of cases (if any) leading to bug 1235159. Is there any real value in doing this or should we aim for better handling of crashes (which is particularly important for other open bugs too).

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> Hmm, actually, is this a good idea at all?
>
> The proposal here only solves a minority of cases (if any) leading to bug
> 1235159. Is there any real value in doing this or should we aim for better
> handling of crashes (which is particularly important for other open bugs too).

I agree it isn't a complete fix but I don't think Mir alone can handle the "kill -9" cases.

OTOH It does address the cases where Mir fails to complete shutdown.

[Alan Griffiths (alan-griffiths)]

Added handling of remaining cases where we know we are exiting.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1111
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/123/
Executed test runs:
    SUCCESS: http://10.97.0.26:8080/job/mir-android-saucy-i386-build/2273
    SUCCESS: http://10.97.0.26:8080/job/mir-clang-saucy-amd64-build/2158
    SUCCESS: http://10.97.0.26:8080/job/mir-team-mir-development-branch-saucy-amd64-ci/120
        deb: http://10.97.0.26:8080/job/mir-team-mir-development-branch-saucy-amd64-ci/120/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/123/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

"kill -9" is irrelevant. All Mir server "crashes" result in catchable signals, upon which we could still do a little recovery.

[Daniel van Vugt (vanvugt)]

This is dangerous:
+ if (sig == SIGSEGV) exit(EXIT_FAILURE);

You shouldn't turn a crash into a clean exit. Or else your segfaults will never generate any crash reports and core files. You should instead re-raise as a fatal signal (like SIGSEGV again, or SIGABRT).

Also, other fatal signals we care about: SIGBUS, SIGPIPE.

review: Needs Fixing

[Eleni Maria Stea (hikiko)]

It looks good. But yes, you have to avoid the clean exit (I don't know if raising another signal is the right approach though so: abstain :))

review: Abstain

[Daniel van Vugt (vanvugt)]

Sorry, I'm out of time today to give this an adequate review.

review: Abstain

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1114
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/131/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2284
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2169
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/128
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/128/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/131/rebuild

review: Approve (continuous-integration)

[Eleni Maria Stea (hikiko)]

I sent the signals one by one and it works fine, I see no /tmp/mir_socket anymore (only in case of kill -9 where we cant really do anything). I'll try to cause a segmentation fault now, it will be interesting to see what happens. Approved :)

review: Approve

[Alexandros Frantzis (afrantzis)]

I don't think we shouldn't be handling non-fatal signals like SIGPIPE, SIGALRM. If other code in the same process as us depends on these (which is not unreasonable), we will remove the endpoint prematurely. I would also argue against including SIGILL, since I know it is used by libcrypto to check for available special instruction on ARM, and libcrypto is used by the unity8 process on the phone.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

After discussion with Alexandros on #ubuntu-mir we agreed on handling: SIGQUIT, SIGABRT, SIGFPE, SIGSEGV, SIGBUS

The mechanism introduced is easily extensible. If someone has an argument for handling additional signals they can propose that in a separate MP.

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1116
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/133/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2287
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2172
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/130
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/130/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/133/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

good

review: Approve

[Daniel van Vugt (vanvugt)]

Turning signals without handlers into an exit is a serious mistake:

117 + if (!old_handler[sig])
118 + {
119 + _Exit(EXIT_FAILURE);
120 + }

Your debugger (or whatever) won't ever know a crash occurred. No crash report generated. The process just vanishes without a (stack)trace. I just tested this by inserting a SIGSEGV and no core was generated, just:
[1]+ Exit 1 sudo bin/mir_demo_server_shell

Instead you should re-raise, and let the system default handler take over:
    raise(sig);
So the system can actually detect the crash, get a core dump and send us crash reports.

Secondly, this "handler" needs a more descriptive name like "handle_fatal_signal". Because developers will see it in stack traces and need to better recognise and understand that's happening:

113 +extern "C" void handler(int sig)

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

P.S. SIGSPIPE is a fatal signal and should be handled (see "man 7 signal").

Also, when testing core dumping remember to "ulimit -c unlimited" first.

[Alan Griffiths (alan-griffiths)]

> P.S. SIGSPIPE is a fatal signal and should be handled (see "man 7 signal").

SIGPIPE is "Term", not "Core" and, I think, correctly handled elsewhere.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1117
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/140/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2300
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2185
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/137
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/137/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/140/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

OK, manual testing by inserting real crashes into the server works. And I get usable core files with usable stack traces. Interestingly, the stack ends at the error location and fatal_signal_cleanup() is never mentioned, despite definitely executing before dumping core. That's strange, but OK.

Testing with fatal exceptions also works, but the resulting core dump has no usable stack trace. I suspect that's a pre-existing issue with C++ exceptions, or at least how we use them?...

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> Testing with fatal exceptions also works, but the resulting core dump has no
> usable stack trace. I suspect that's a pre-existing issue with C++ exceptions,
> or at least how we use them?...

The standard doesn't specify whether, with an unhandled exception, the stack is unwound before calling terminate(). g++ does the unwinding (which destroys the stack trace).

The real problem is folks starting threads without exception handlers where needed.

Which thread(s) are you creating these fatal exceptions on?

[Daniel van Vugt (vanvugt)]

Nevermind. Confirmed the lack of stack trace on fatal exceptions is indeed a pre-existing problem.

review: Approve

[Daniel van Vugt (vanvugt)]

Approved. Although a parting thought...

delete_endpoint should simply be called "cleanup" or something generic, so we can add more logic like DRM cleanup (to resolve an open bug or two). It should also have a comment mentioning it might be called as the result of a crash, and so _must_not_ use any heap operations.

[Daniel van Vugt (vanvugt)]

My concerns about exceptions are logged here --> bug 1237332

[Alexandros Frantzis (afrantzis)]

From POSIX.1-2008:

"The ISO C standard places a restriction on applications relating to the use of raise() from signal handlers. This restriction does not apply to POSIX applications, as POSIX.1-2008 requires raise() to be async-signal-safe"

(referring to nested raise() calls)

glibc follows POSIX.1-2008, so everything should be fine.

review: Approve

=== modified file 'examples/render_surfaces.cpp'
--- examples/render_surfaces.cpp	2013-09-24 16:22:46 +0000
+++ examples/render_surfaces.cpp	2013-10-09 08:48:59 +0000
@@ -273,6 +273,7 @@
             void start() {}
             void stop() {}
             int client_socket_fd() const override { return 0; }
+            void remove_endpoint() const override { }
         };
 
         return std::make_shared<NullConnector>();

=== modified file 'include/server/mir/frontend/connector.h'
--- include/server/mir/frontend/connector.h	2013-09-24 14:02:58 +0000
+++ include/server/mir/frontend/connector.h	2013-10-09 08:48:59 +0000
@@ -31,6 +31,7 @@
     virtual void stop() = 0;
 
     virtual int client_socket_fd() const = 0;
+    virtual void remove_endpoint() const = 0;
 
 protected:
     Connector() = default;

=== modified file 'src/server/frontend/published_socket_connector.cpp'
--- src/server/frontend/published_socket_connector.cpp	2013-09-27 16:04:11 +0000
+++ src/server/frontend/published_socket_connector.cpp	2013-10-09 08:48:59 +0000
@@ -46,7 +46,7 @@
 
 mf::PublishedSocketConnector::~PublishedSocketConnector() noexcept
 {
-    std::remove(socket_file.c_str());
+    remove_endpoint();
 }
 
 void mf::PublishedSocketConnector::start_accept()
@@ -64,6 +64,11 @@
             ba::placeholders::error));
 }
 
+void mf::PublishedSocketConnector::remove_endpoint() const
+{
+    std::remove(socket_file.c_str());
+}
+
 void mf::PublishedSocketConnector::on_new_connection(
     std::shared_ptr<boost::asio::local::stream_protocol::socket> const& socket,
     boost::system::error_code const& ec)
@@ -166,6 +171,10 @@
     return socket_fd[client];
 }
 
+void mf::BasicConnector::remove_endpoint() const
+{
+}
+
 mf::BasicConnector::~BasicConnector() noexcept
 {
     stop();

=== modified file 'src/server/frontend/published_socket_connector.h'
--- src/server/frontend/published_socket_connector.h	2013-09-27 16:04:11 +0000
+++ src/server/frontend/published_socket_connector.h	2013-10-09 08:48:59 +0000
@@ -56,6 +56,8 @@
     void start() override;
     void stop() override;
     int client_socket_fd() const override;
+    void remove_endpoint() const override;
+
 
 protected:
     void create_session_for(std::shared_ptr<boost::asio::local::stream_protocol::socket> const& server_socket) const;
@@ -80,6 +82,8 @@
         std::shared_ptr<ConnectorReport> const& report);
     ~PublishedSocketConnector() noexcept;
 
+    void remove_endpoint() const override;
+
 private:
     void start_accept();
     void on_new_connection(std::shared_ptr<boost::asio::local::stream_protocol::socket> const& socket,

=== modified file 'src/server/run_mir.cpp'
--- src/server/run_mir.cpp	2013-04-24 05:22:20 +0000
+++ src/server/run_mir.cpp	2013-10-09 08:48:59 +0000
@@ -20,10 +20,38 @@
 #include "mir/display_server.h"
 #include "mir/main_loop.h"
 #include "mir/server_configuration.h"
+#include "mir/frontend/connector.h"
 
 #include <csignal>
+#include <cstdlib>
 #include <cassert>
 
+namespace
+{
+std::weak_ptr<mir::frontend::Connector> weak_connector;
+
+extern "C" void delete_endpoint()
+{
+    if (auto connector = weak_connector.lock())
+    {
+        weak_connector.reset();
+        connector->remove_endpoint();
+    }
+}
+
+extern "C" { typedef void (*sig_handler)(int); }
+
+volatile sig_handler old_handler[SIGUNUSED]  = { nullptr };
+
+extern "C" void fatal_signal_cleanup(int sig)
+{
+    delete_endpoint();
+
+    signal(sig, old_handler[sig]);
+    raise(sig);
+}
+}
+
 void mir::run_mir(ServerConfiguration& config, std::function<void(DisplayServer&)> init)
 {
     DisplayServer* server_ptr{nullptr};
@@ -33,6 +61,7 @@
         {SIGINT, SIGTERM},
         [&server_ptr](int)
         {
+            delete_endpoint();
             assert(server_ptr);
             server_ptr->stop();
         });
@@ -40,6 +69,13 @@
     DisplayServer server(config);
     server_ptr = &server;
 
+    weak_connector = config.the_connector();
+
+    for (auto sig : { SIGQUIT, SIGABRT, SIGFPE, SIGSEGV, SIGBUS })
+        old_handler[sig] = signal(sig, fatal_signal_cleanup);
+
+    std::atexit(&delete_endpoint);
+
     init(server);
     server.run();
 }

=== modified file 'tests/acceptance-tests/test_server_shutdown.cpp'
--- tests/acceptance-tests/test_server_shutdown.cpp	2013-09-27 16:59:05 +0000
+++ tests/acceptance-tests/test_server_shutdown.cpp	2013-10-09 08:48:59 +0000
@@ -112,7 +112,9 @@
 
 }
 
-TEST_F(BespokeDisplayServerTestFixture, server_can_shut_down_when_clients_are_blocked)
+using ServerShutdown = BespokeDisplayServerTestFixture;
+
+TEST_F(ServerShutdown, server_can_shut_down_when_clients_are_blocked)
 {
     Flag next_buffer_done1{"next_buffer_done1_c5d49978.tmp"};
     Flag next_buffer_done2{"next_buffer_done2_c5d49978.tmp"};
@@ -199,7 +201,7 @@
     });
 }
 
-TEST_F(BespokeDisplayServerTestFixture, server_releases_resources_on_shutdown_with_connected_clients)
+TEST_F(ServerShutdown, server_releases_resources_on_shutdown_with_connected_clients)
 {
     Flag surface_created1{"surface_created1_7e9c69fc.tmp"};
     Flag surface_created2{"surface_created2_7e9c69fc.tmp"};
@@ -341,3 +343,91 @@
         resources_freed_success.set();
     }
 }
+
+namespace
+{
+bool file_exists(std::string const& filename)
+{
+    struct stat statbuf;
+    return 0 == stat(filename.c_str(), &statbuf);
+}
+}
+
+TEST_F(ServerShutdown, server_removes_endpoint_on_normal_exit)
+{
+    using ServerConfig = TestingServerConfiguration;
+
+    ServerConfig server_config;
+    launch_server_process(server_config);
+
+    run_in_test_process([&]
+    {
+        ASSERT_TRUE(file_exists(server_config.the_socket_file()));
+
+        shutdown_server_process();
+        EXPECT_FALSE(file_exists(server_config.the_socket_file()));
+    });
+}
+
+TEST_F(ServerShutdown, server_removes_endpoint_on_abort)
+{
+    struct ServerConfig : TestingServerConfiguration
+    {
+        void exec() override
+        {
+            abort();
+        }
+    };
+
+    ServerConfig server_config;
+    launch_server_process(server_config);
+
+    run_in_test_process([&]
+    {
+        using std::chrono::steady_clock;
+        std::chrono::seconds const limit(2);
+
+        ASSERT_TRUE(file_exists(server_config.the_socket_file()));
+
+        // shutdown should fail as the server aborted
+        ASSERT_FALSE(shutdown_server_process());
+
+        EXPECT_FALSE(file_exists(server_config.the_socket_file()));
+    });
+}
+
+struct OnSignal : ServerShutdown, ::testing::WithParamInterface<int> {};
+
+TEST_P(OnSignal, removes_endpoint_on_signal)
+{
+    struct ServerConfig : TestingServerConfiguration
+    {
+        void exec() override
+        {
+            raise(sig);
+        }
+
+        ServerConfig(int sig) : sig(sig) {}
+        int const sig;
+    };
+
+    ServerConfig server_config(GetParam());
+    launch_server_process(server_config);
+
+    run_in_test_process([&]
+    {
+        using std::chrono::steady_clock;
+        std::chrono::seconds const limit(2);
+
+        ASSERT_TRUE(file_exists(server_config.the_socket_file()));
+
+        // shutdown should fail as the server faulted
+        ASSERT_FALSE(shutdown_server_process());
+
+        EXPECT_FALSE(file_exists(server_config.the_socket_file()));
+    });
+}
+
+INSTANTIATE_TEST_CASE_P(ServerShutdown,
+    OnSignal,
+    ::testing::Values(SIGQUIT, SIGABRT, SIGFPE, SIGSEGV, SIGBUS));

=== modified file 'tests/integration-tests/shell/test_session.cpp'
--- tests/integration-tests/shell/test_session.cpp	2013-09-24 16:22:46 +0000
+++ tests/integration-tests/shell/test_session.cpp	2013-10-09 08:48:59 +0000
@@ -68,6 +68,7 @@
             void start() {}
             void stop() {}
             int client_socket_fd() const override { return 0; }
+            void remove_endpoint() const override { }
         };
 
         return std::make_shared<NullConnector>();

=== modified file 'tests/integration-tests/test_display_server_main_loop_events.cpp'
--- tests/integration-tests/test_display_server_main_loop_events.cpp	2013-09-24 14:02:58 +0000
+++ tests/integration-tests/test_display_server_main_loop_events.cpp	2013-10-09 08:48:59 +0000
@@ -59,6 +59,7 @@
     MOCK_METHOD0(start, void());
     MOCK_METHOD0(stop, void());
     MOCK_CONST_METHOD0(client_socket_fd, int());
+    MOCK_CONST_METHOD0(remove_endpoint, void());
 };
 
 class MockDisplayChanger : public mir::DisplayChanger