Merge lp:~alan-griffiths/miral/fix-1668651 into lp:miral

[Alan Griffiths]

miral-shell shutdown was a mess. Fixed.

miral-shell shutdown was a mess.

After fixing the segfault, the server closed down first leading to SIGHUP causing the server to shutdown with EXIT_FAILURE.

So changed the lifecycle callback.

Then operations on the decorations "client" were failing (because the connection had closed). So made it possible for the shell main() to join the client threads.

The server now closes cleanly.

Not sure this is the cleanest code, so any suggestions are welcome.

[Alan Griffiths (alan-griffiths)]

Note that on versions of Mir prior to 0.24 closing the server with a signal (e.g. SIGTERM) still leads to a segfault. That's because we lack mir::server::add_stop_callback() - or any way to hook into that mechanism for closing the server.

That affects 16.04LTS (Mir 0.21) but is "only" a crash on exit. I don't think we need to block. (But I'll think about possible solutions.)

[Alan Griffiths (alan-griffiths)]

> That affects 16.04LTS (Mir 0.21) but is "only" a crash on exit. I don't think
> we need to block. (But I'll think about possible solutions.)

Found a solution

[Alan Griffiths (alan-griffiths)]

I've considered maintaining an internal list of InternalClientRunners and calling join_client_thread() on all of them immediately *after* executing any client provided stop callbacks.

That would simplify the main() functions (miral-kiosk needs nothing, miral-shell just needs the hook to close the decorations provider).

It would also be possible to introduce a mechanism to notify internal clients of a close request by supplying overloads that provide a callback for this purpose.

But all that is providing convenience for simple usecases when the mechanisms provided here are needed to address the general case.

[Gerry Boland (gerboland)]

All looks ok!

review: Approve

[Gerry Boland (gerboland)]

Ok, this makes sense.

review: Approve

=== modified file 'miral-shell/decoration_provider.cpp'
--- miral-shell/decoration_provider.cpp	2017-02-24 13:00:28 +0000
+++ miral-shell/decoration_provider.cpp	2017-03-02 13:47:39 +0000
@@ -258,7 +258,6 @@
 
 DecorationProvider::~DecorationProvider()
 {
-    stop();
 }
 
 void DecorationProvider::stop()
@@ -271,16 +270,18 @@
 
     enqueue_work([this]
         {
-             {
+            if (connection)
+            {
 #if MIR_CLIENT_API_VERSION < MIR_VERSION_NUMBER(0, 26, 2)
                 auto const Workaround_lp_1667645 =
-                     WindowSpec::for_normal_window(connection, 100, 100, mir_pixel_format_xrgb_8888).create_window();
+                     WindowSpec::for_normal_window(connection, 100, 100, mir_pixel_format_xrgb_8888)
+                         .set_name(wallpaper_name).create_window();
 #endif
                 wallpaper.erase(begin(wallpaper), end(wallpaper));
-             }
+            }
             connection.reset();
-            stop_work();
         });
+    stop_work();
 }
 
 namespace
@@ -423,7 +424,7 @@
 void DecorationProvider::place_new_decoration(miral::WindowSpecification& window_spec)
 {
     auto const name = window_spec.name().value();
-    if (name == "wallpaper") return;
+    if (name == wallpaper_name) return;
 
     std::lock_guard<decltype(mutex)> lock{mutex};
 
@@ -532,7 +533,6 @@
 
 Worker::~Worker()
 {
-    if (worker.joinable()) worker.join();
 }
 
 void Worker::do_work()
@@ -560,7 +560,7 @@
 
 void Worker::start_work()
 {
-    worker = std::thread{[this] { do_work(); }};
+    do_work();
 }
 
 void Worker::stop_work()

=== modified file 'miral-shell/decoration_provider.h'
--- miral-shell/decoration_provider.h	2017-02-21 11:54:06 +0000
+++ miral-shell/decoration_provider.h	2017-03-02 13:47:39 +0000
@@ -31,7 +31,6 @@
 #include <atomic>
 #include <map>
 #include <mutex>
-#include <thread>
 #include <condition_variable>
 #include <queue>
 
@@ -51,7 +50,6 @@
     std::condition_variable work_cv;
     WorkQueue work_queue;
     bool work_done = false;
-    std::thread worker;
 
     void do_work();
 };

=== modified file 'miral-shell/shell_main.cpp'
--- miral-shell/shell_main.cpp	2016-12-01 11:48:56 +0000
+++ miral-shell/shell_main.cpp	2017-03-02 13:47:39 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2016 Canonical Ltd.
+ * Copyright © 2016-2017 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
@@ -37,17 +37,21 @@
 {
     using namespace miral;
 
+    std::function<void()> shutdown_hook{[]{}};
+
     SpinnerSplash spinner;
     InternalClientLauncher launcher;
     ActiveOutputsMonitor outputs_monitor;
     WindowManagerOptions window_managers
         {
-            add_window_manager_policy<TitlebarWindowManagerPolicy>("titlebar", spinner, launcher),
+            add_window_manager_policy<TitlebarWindowManagerPolicy>("titlebar", spinner, launcher, shutdown_hook),
             add_window_manager_policy<TilingWindowManagerPolicy>("tiling", spinner, launcher, outputs_monitor),
         };
 
     MirRunner runner{argc, argv};
 
+    runner.add_stop_callback([&] { shutdown_hook(); });
+
     auto const quit_on_ctrl_alt_bksp = [&](MirEvent const* event)
         {
             if (mir_event_get_type(event) != mir_event_type_input)

=== modified file 'miral-shell/titlebar_window_manager.cpp'
--- miral-shell/titlebar_window_manager.cpp	2017-02-21 11:54:06 +0000
+++ miral-shell/titlebar_window_manager.cpp	2017-03-02 13:47:39 +0000
@@ -49,12 +49,14 @@
 TitlebarWindowManagerPolicy::TitlebarWindowManagerPolicy(
     WindowManagerTools const& tools,
     SpinnerSplash const& spinner,
-    miral::InternalClientLauncher const& launcher) :
+    miral::InternalClientLauncher const& launcher,
+    std::function<void()>& shutdown_hook) :
     CanonicalWindowManagerPolicy(tools),
     spinner{spinner},
     decoration_provider{std::make_unique<DecorationProvider>(tools)}
 {
     launcher.launch("decorations", *decoration_provider);
+    shutdown_hook = [this] { decoration_provider->stop(); };
 
     for (auto key : {KEY_F1, KEY_F2, KEY_F3, KEY_F4})
         key_to_workspace[key] = this->tools.create_workspace();
@@ -497,14 +499,6 @@
         }
     }
 
-    // TODO this is a workaround for the lack of a way to detect server exit (Mir bug lp:1593655)
-    // We need to exit the titlebar_provider "client" thread before the server exits
-    if (action == mir_keyboard_action_down && scan_code == KEY_BACKSPACE &&
-        (modifiers == (mir_input_event_modifier_alt | mir_input_event_modifier_ctrl)))
-    {
-        decoration_provider->stop();
-    }
-
     return false;
 }
 

=== modified file 'miral-shell/titlebar_window_manager.h'
--- miral-shell/titlebar_window_manager.h	2017-02-21 11:54:06 +0000
+++ miral-shell/titlebar_window_manager.h	2017-03-02 13:47:39 +0000
@@ -36,7 +36,11 @@
 class TitlebarWindowManagerPolicy : public miral::CanonicalWindowManagerPolicy, miral::WorkspacePolicy
 {
 public:
-    TitlebarWindowManagerPolicy(miral::WindowManagerTools const& tools, SpinnerSplash const& spinner, miral::InternalClientLauncher const& launcher);
+    TitlebarWindowManagerPolicy(
+        miral::WindowManagerTools const& tools,
+        SpinnerSplash const& spinner,
+        miral::InternalClientLauncher const& launcher,
+        std::function<void()>& shutdown_hook);
     ~TitlebarWindowManagerPolicy();
 
     virtual miral::WindowSpecification place_new_window(

=== modified file 'miral/CMakeLists.txt'
--- miral/CMakeLists.txt	2017-02-20 17:31:11 +0000
+++ miral/CMakeLists.txt	2017-03-02 13:47:39 +0000
@@ -21,7 +21,8 @@
     window_management_trace.cpp         window_management_trace.h
     xcursor_loader.cpp                  xcursor_loader.h
     xcursor.c                           xcursor.h
-    both_versions.h
+                                        both_versions.h
+                                        join_client_threads.h
 )
 
 set_source_files_properties(xcursor.c PROPERTIES COMPILE_DEFINITIONS _GNU_SOURCE)

=== modified file 'miral/internal_client.cpp'
--- miral/internal_client.cpp	2017-02-15 12:17:12 +0000
+++ miral/internal_client.cpp	2017-03-02 13:47:39 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2016 Canonical Ltd.
+ * Copyright © 2016-2017 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 3,
@@ -17,6 +17,7 @@
  */
 
 #include "miral/internal_client.h"
+#include "join_client_threads.h"
 #include "both_versions.h"
 
 #include <mir/fd.h>
@@ -24,10 +25,14 @@
 #include <mir/scene/session.h>
 #include <mir/main_loop.h>
 
+#define MIR_LOG_COMPONENT "miral::Internal Client"
+#include <mir/log.h>
+
 #include <atomic>
 #include <condition_variable>
 #include <mutex>
 #include <thread>
+#include <map>
 
 namespace
 {
@@ -39,6 +44,8 @@
          std::function<void(std::weak_ptr<mir::scene::Session> const session)> connect_notification);
 
     void run(mir::Server& server);
+    void join_client_thread();
+
     ~InternalClientRunner();
 
 private:
@@ -52,13 +59,34 @@
     std::function<void(mir::client::Connection connection)> const client_code;
     std::function<void(std::weak_ptr<mir::scene::Session> const session)> connect_notification;
 };
-}
-
-class miral::StartupInternalClient::Self : InternalClientRunner
-{
-public:
+
+std::mutex client_runners_mutex;
+std::multimap<mir::Server*, std::weak_ptr<InternalClientRunner>> client_runners;
+
+void register_runner(mir::Server* server, std::weak_ptr<InternalClientRunner> internal_client)
+{
+    std::lock_guard<decltype(client_runners_mutex)> lock{client_runners_mutex};
+    client_runners.emplace(server, std::move(internal_client));
+}
+
+void join_runners_for(mir::Server* server)
+{
+    std::lock_guard<decltype(client_runners_mutex)> lock{client_runners_mutex};
+    auto range = client_runners.equal_range(server);
+
+    for (auto i = range.first; i != range.second; ++i)
+    {
+        if (auto runner = i->second.lock())
+            runner->join_client_thread();
+    }
+
+    client_runners.erase(range.first, range.second);
+}
+}
+
+class miral::StartupInternalClient::Self : public InternalClientRunner
+{
     using InternalClientRunner::InternalClientRunner;
-    using InternalClientRunner::run;
 };
 
 InternalClientRunner::InternalClientRunner(
@@ -86,6 +114,16 @@
 
     connection = mir::client::Connection{mir_connect_sync(connect_string, name.c_str())};
 
+    mir_connection_set_lifecycle_event_callback(
+        connection,
+        [](MirConnection*, MirLifecycleState transition, void*)
+        {
+            // The default handler kills the process with SIGHUP - which is unhelpful for internal clients
+            if (transition == mir_lifecycle_connection_lost)
+                mir::log_warning("server disconnected before connection released by client");
+        },
+        this);
+
     std::unique_lock<decltype(mutex)> lock{mutex};
     cv.wait(lock, [&] { return !!session.lock(); });
 
@@ -98,6 +136,12 @@
 
 InternalClientRunner::~InternalClientRunner()
 {
+    join_client_thread();
+}
+
+
+void InternalClientRunner::join_client_thread()
+{
     if (thread.joinable())
     {
         thread.join();
@@ -124,6 +168,8 @@
 
 void miral::StartupInternalClient::operator()(mir::Server& server)
 {
+    register_runner(&server, internal_client);
+
     server.add_init_callback([this, &server]
     {
         server.the_main_loop()->enqueue(this, [this, &server]
@@ -138,7 +184,7 @@
 struct miral::InternalClientLauncher::Self
 {
     mir::Server* server = nullptr;
-    std::unique_ptr<InternalClientRunner> runner;
+    std::shared_ptr<InternalClientRunner> runner;
 };
 
 void miral::InternalClientLauncher::operator()(mir::Server& server)
@@ -155,9 +201,15 @@
     std::function<void(mir::client::Connection connection)> const& client_code,
     std::function<void(std::weak_ptr<mir::scene::Session> const session)> const& connect_notification) const
 {
-    self->runner = std::make_unique<InternalClientRunner>(name, client_code, connect_notification);
+    self->runner = std::make_shared<InternalClientRunner>(name, client_code, connect_notification);
     self->server->the_main_loop()->enqueue(this, [this] { self->runner->run(*self->server); });
+    register_runner(self->server, self->runner);
 }
 
 miral::InternalClientLauncher::InternalClientLauncher() : self{std::make_shared<Self>()} {}
 miral::InternalClientLauncher::~InternalClientLauncher() = default;
+
+void join_client_threads(mir::Server* server)
+{
+    join_runners_for(server);
+}

=== added file 'miral/join_client_threads.h'
--- miral/join_client_threads.h	1970-01-01 00:00:00 +0000
+++ miral/join_client_threads.h	2017-03-02 13:47:39 +0000
@@ -0,0 +1,24 @@
+/*
+ * Copyright © 2017 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#ifndef MIRAL_JOIN_CLIENT_THREADS_H
+#define MIRAL_JOIN_CLIENT_THREADS_H
+
+void join_client_threads(mir::Server* server);
+
+#endif //MIRAL_JOIN_CLIENT_THREADS_H

=== modified file 'miral/runner.cpp'
--- miral/runner.cpp	2016-12-01 11:48:56 +0000
+++ miral/runner.cpp	2017-03-02 13:47:39 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2016 Canonical Ltd.
+ * Copyright © 2016-2017 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 3,
@@ -17,6 +17,7 @@
  */
 
 #include "miral/runner.h"
+#include "join_client_threads.h"
 
 #include <mir/server.h>
 #include <mir/main_loop.h>
@@ -29,6 +30,10 @@
 #include <mutex>
 #include <thread>
 
+#if MIR_SERVER_VERSION < MIR_VERSION_NUMBER(0, 24, 0)
+#include <csignal>
+#endif
+
 namespace
 {
 inline auto filename(std::string path) -> std::string
@@ -50,7 +55,7 @@
     
     std::mutex mutex;
     std::function<void()> start_callback{[]{}};
-    std::function<void()> stop_callback{[]{}};
+    std::function<void()> stop_callback{[this]{ join_client_threads(weak_server.lock().get()); }};
     std::function<void()> exception_handler{static_cast<void(*)()>(mir::report_exception)};
     std::weak_ptr<mir::Server> weak_server;
 };
@@ -215,6 +220,10 @@
         auto const main_loop = server->the_main_loop();
         main_loop->enqueue(this, start_callback);
 
+#if MIR_SERVER_VERSION < MIR_VERSION_NUMBER(0, 24, 0)
+        main_loop->register_signal_handler({SIGINT, SIGTERM}, [this](int) {stop_callback();});
+#endif
+
         server->run();
     }
 

=== modified file 'miral/symbols.map'
--- miral/symbols.map	2017-02-15 17:32:34 +0000
+++ miral/symbols.map	2017-03-02 13:47:39 +0000
@@ -381,17 +381,10 @@
     miral::WorkspacePolicy::advise_adding_to_workspace*;
     miral::WorkspacePolicy::advise_removing_from_workspace*;
     miral::WorkspacePolicy::operator*;
-    miral::toolkit::Window::Window*;
     non-virtual?thunk?to?miral::WorkspacePolicy::?WorkspacePolicy*;
     non-virtual?thunk?to?miral::WorkspacePolicy::advise_adding_to_workspace*;
     non-virtual?thunk?to?miral::WorkspacePolicy::advise_removing_from_workspace*;
     typeinfo?for?miral::WorkspacePolicy;
-    typeinfo?for?miral::toolkit::Window;
-    typeinfo?for?miral::toolkit::WindowId;
-    typeinfo?for?miral::toolkit::WindowSpec;
     vtable?for?miral::WorkspacePolicy;
-    vtable?for?miral::toolkit::Window;
-    vtable?for?miral::toolkit::WindowId;
-    vtable?for?miral::toolkit::WindowSpec;
   };
 } MIRAL_1.2;