Merge lp:~mir-team/mir/fix-delayed-sigaction-restore into lp:mir

[Andreas Pokorny]

Keep sigaction alive through shared_ptr

This is the MP with the glib problems

Unmerged revisions

2318. By Andreas Pokorny on 2015-02-16

Keep sig actions alive through shared_ptr

=== modified file 'src/include/server/mir/glib_main_loop_sources.h'
--- src/include/server/mir/glib_main_loop_sources.h	2015-02-12 02:11:25 +0000
+++ src/include/server/mir/glib_main_loop_sources.h	2015-02-16 17:20:22 +0000
@@ -104,6 +104,7 @@
 
 private:
     class SourceRegistration;
+    class RegisteredSigAction;
     struct HandlerElement
     {
         operator bool() const { return !!handler; }
@@ -121,7 +122,7 @@
     mir::Fd signal_write_fd;
     mir::ThreadSafeList<HandlerElement> handlers;
     std::mutex handled_signals_mutex;
-    std::unordered_map<int, struct sigaction> handled_signals;
+    std::vector<std::shared_ptr<RegisteredSigAction>> handled_signals;
     std::unique_ptr<SourceRegistration> source_registration;
 };
 
 
=== modified file 'src/server/glib_main_loop_sources.cpp'
--- src/server/glib_main_loop_sources.cpp	2015-02-12 02:11:25 +0000
+++ src/server/glib_main_loop_sources.cpp	2015-02-16 17:20:22 +0000
@@ -24,6 +24,7 @@
 #include <atomic>
 #include <system_error>
 #include <sstream>
+#include <iostream>
 
 #include <csignal>
 #include <unistd.h>
@@ -494,6 +495,56 @@
 
 std::array<std::atomic<int>,10> md::SignalSources::SourceRegistration::write_fds;
 
+struct md::SignalSources::RegisteredSigAction
+{
+    int sig;
+    struct sigaction old_action;
+
+    RegisteredSigAction(RegisteredSigAction const&) = delete;
+    RegisteredSigAction& operator=(RegisteredSigAction const&) = delete;
+
+    RegisteredSigAction(int sig)
+        : sig{sig}
+    {
+        int const no_flags{0};
+        struct sigaction new_action;
+
+        new_action.sa_handler = SourceRegistration::notify_sources_of_signal;
+        sigfillset(&new_action.sa_mask);
+        new_action.sa_flags = no_flags;
+
+        if (sigaction(sig, &new_action, &old_action) == -1)
+        {
+            std::stringstream msg;
+            msg << "Failed to register action for signal " << sig;
+            BOOST_THROW_EXCEPTION(
+                std::system_error(errno, std::system_category(), msg.str()));
+        }
+    }
+    ~RegisteredSigAction()
+    {
+        std::unique_lock<std::mutex> lock(sig_registry_mutex);
+
+        sigaction(sig, &old_action, nullptr);
+    }
+    static std::shared_ptr<RegisteredSigAction> register_action(int sig)
+    {
+        std::unique_lock<std::mutex> lock(sig_registry_mutex);
+
+        auto & entry = sig_registry[sig];
+        auto ret = entry.lock();
+        if (!ret)
+            entry = ret = std::make_shared<RegisteredSigAction>(sig);
+
+        return ret;
+    }
+    static std::mutex sig_registry_mutex;
+    static std::unordered_map<int, std::weak_ptr<RegisteredSigAction>> sig_registry;
+};
+
+std::mutex md::SignalSources::RegisteredSigAction::sig_registry_mutex;
+std::unordered_map<int, std::weak_ptr<md::SignalSources::RegisteredSigAction>> md::SignalSources::RegisteredSigAction::sig_registry;
+
 md::SignalSources::SignalSources(md::FdSources& fd_sources)
     : fd_sources(fd_sources)
 {
@@ -521,9 +572,6 @@
 
 md::SignalSources::~SignalSources()
 {
-    for (auto const& handled : handled_signals)
-        sigaction(handled.first, &handled.second, nullptr);
-
     fd_sources.remove_all_owned_by(this);
 }
 
@@ -573,26 +621,7 @@
 {
     std::lock_guard<std::mutex> lock{handled_signals_mutex};
 
-    if (handled_signals.find(sig) != handled_signals.end())
-        return;
-
-    static int const no_flags{0};
-    struct sigaction old_action;
-    struct sigaction new_action;
-
-    new_action.sa_handler = SourceRegistration::notify_sources_of_signal;
-    sigfillset(&new_action.sa_mask);
-    new_action.sa_flags = no_flags;
-
-    if (sigaction(sig, &new_action, &old_action) == -1)
-    {
-        std::stringstream msg;
-        msg << "Failed to register action for signal " << sig;
-        BOOST_THROW_EXCEPTION(
-            std::system_error(errno, std::system_category(), msg.str()));
-    }
-
-    handled_signals.emplace(sig, old_action);
+    handled_signals.emplace_back(RegisteredSigAction::register_action(sig));
 }
 
 void md::SignalSources::dispatch_signal(int sig)
 
=== modified file 'tests/unit-tests/test_glib_main_loop.cpp'
--- tests/unit-tests/test_glib_main_loop.cpp	2015-02-03 15:07:44 +0000
+++ tests/unit-tests/test_glib_main_loop.cpp	2015-02-16 17:20:22 +0000
@@ -1078,3 +1078,103 @@
     // doesn't handle signals, and thus hangs forever.
     execute_in_forked_process(this, [&] { check_mainloop_signal_handling(); });
 }
+
+TEST(GLibMainLoopRaceTest, signal_handlers_do_not_race_for_destruction)
+{
+    using namespace ::testing;
+
+    for (int i = 0; i != 100; ++i)
+    {
+        int const signum = SIGUSR1;
+        int handled_signum = 0;
+        mt::Signal first_loop_running;
+        mt::Signal first_loop_finished;
+        mt::Signal first_loop_received_signal;
+        mt::Signal second_loop_running;
+        mt::Signal second_loop_finished;
+
+        {
+            mir::GLibMainLoop first{std::make_shared<mir::time::SteadyClock>()};
+            mir::GLibMainLoop second{std::make_shared<mir::time::SteadyClock>()};
+
+            first.register_signal_handler(
+                {signum},
+                [&](int)
+                {
+                });
+
+
+            std::thread{
+                [&]
+                {
+                    int const owner{0};
+                    first.enqueue(&owner, [&] { first_loop_running.raise(); });
+                    first.run();
+                    first_loop_finished.raise();
+                }}.detach();
+
+            std::thread{
+                [&]
+                {
+                    int const owner{0};
+                    second.enqueue(&owner, [&] { second_loop_running.raise(); });
+                    second.run();
+                    second_loop_finished.raise();
+                }}.detach();
+
+                    ASSERT_TRUE(first_loop_running.wait_for(std::chrono::seconds{5}));
+            ASSERT_TRUE(second_loop_running.wait_for(std::chrono::seconds{5}));
+
+
+            first.stop();
+            second.stop();
+
+            EXPECT_TRUE(first_loop_finished.wait_for(std::chrono::seconds{5}));
+            EXPECT_TRUE(second_loop_finished.wait_for(std::chrono::seconds{5}));
+        }
+
+        {
+            mir::GLibMainLoop first{std::make_shared<mir::time::SteadyClock>()};
+            mir::GLibMainLoop second{std::make_shared<mir::time::SteadyClock>()};
+            first.register_signal_handler(
+                        {signum},
+                        [&](int sig)
+                        {
+                            handled_signum = sig;
+                            first_loop_received_signal.raise();
+                        });
+
+            std::thread{
+                [&]
+                {
+                    int const owner{0};
+                    first.enqueue(&owner, [&] { first_loop_running.raise(); });
+                    first.run();
+                    first_loop_finished.raise();
+                }}.detach();
+
+            std::thread{
+                [&]
+                {
+                    int const owner{0};
+                    second.enqueue(&owner, [&] { second_loop_running.raise(); });
+                    second.run();
+                    second_loop_finished.raise();
+                }}.detach();
+
+
+            ASSERT_TRUE(first_loop_running.wait_for(std::chrono::seconds{5}));
+            ASSERT_TRUE(second_loop_running.wait_for(std::chrono::seconds{5}));
+            kill(getpid(), signum);
+
+            ASSERT_TRUE(first_loop_received_signal.wait_for(std::chrono::seconds{5}));
+            first.stop();
+            second.stop();
+
+            EXPECT_TRUE(first_loop_finished.wait_for(std::chrono::seconds{5}));
+            EXPECT_TRUE(second_loop_finished.wait_for(std::chrono::seconds{5}));
+        }
+
+        EXPECT_THAT(handled_signum, Eq(signum));
+    }
+}