Merge lp:~afrantzis/mir/display-server-main-loop-1 into lp:~mir-team/mir/trunk

[Alexandros Frantzis]

server: Use a main loop to handle events in the display server

server: Use a main loop to handle events in the display server

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:570
http://jenkins.qa.ubuntu.com/job/mir-ci/322/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/294
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/177
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/327
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/327/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/135

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

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

LGTM

review: Approve

[Alan Griffiths (alan-griffiths)]

361 + auto asio_event = std::static_pointer_cast<AsioMainLoopEvent>(event);
...
370 + auto asio_event = std::static_pointer_cast<AsioMainLoopEvent>(event);

These casts look wrong - there should not be undefined behaviour if a conforming parameter is supplied.

I'm still trying to decide if the casts should be dynamic, if the function signature is wrong, or if some other way to recover type information is appropriate (e.g. double dispatch).

~~~~

982 +TEST(DisplayServerMainLoopEvents, display_server_shuts_down_properly_on_sigint)
983 +{
984 + ServerConfig server_config;
985 +
986 + mir::run_mir(server_config,
987 + [](mir::DisplayServer&)
988 + {
989 + kill(getpid(), SIGINT);
990 + });
991 +}

Does this really do what it claims? (In run_mir() init() is called before invoking run() - so it may not be the best way to test the required behaviour.)

~~~~

482 + std::shared_ptr<mir::MainLoopFactory> main_loop_factory;

Why does display server need a dependency on MainLoopFactory? AFAICS it just needs a MainLoop.

Can't MainLoopFactory be subsumed into DefaultServerConfiguration?

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> 361 + auto asio_event = std::static_pointer_cast<AsioMainLoopEvent>(event);
> ...
> 370 + auto asio_event = std::static_pointer_cast<AsioMainLoopEvent>(event);
>
> These casts look wrong - there should not be undefined behaviour if a conforming parameter is supplied.
>
> I'm still trying to decide if the casts should be dynamic, if the function signature is wrong, or if some other
> way to recover type information is appropriate (e.g. double dispatch).

The rationale for this is that there should be a single factory in use providing compatible events (i.e. that can be safely downcasted) , but this is indeed a fragile assumption. I am happy to change this to a dynamic_cast (or we can discuss more about the other options).

> Does this really do what it claims? (In run_mir() init() is called before invoking run() -
> so it may not be the best > way to test the required behaviour.

At the moment we don't have a reliable mechanism to detect when the server has actually started. However, we start queuing signal events from the moment we create the DisplayServer, not from the moment we start running it, so the code above will work fine. I considered adding a way to notify the caller that the display server has actually started, e.g., a display_server_started callback, but I didn't want to overload this MP.

> 482 + std::shared_ptr<mir::MainLoopFactory> main_loop_factory;
> Why does display server need a dependency on MainLoopFactory? AFAICS it just needs a MainLoop.
> Can't MainLoopFactory be subsumed into DefaultServerConfiguration?

The Display server needs to create a stop event, which needs a handle to a main loop factory.

Also, in the near future the Platform will need to be able to create a platform events (e.g. PauseResume), which again need the main loop factory. The main loop factory can be supplied either at Platform construction time, or at event creation time(platform->create_bla_event(factory)).

So one option is:

1. DisplayConfiguration::the_main_loop_factory()
2. DisplayServer uses the main loop factory to create the stop event
3. DisplayServer passes the main loop factory into platform->create_bla_event(factory) to create platform events

Alternatively the main loop factory can be subsumed into the configuration:

1. DisplayConfiguration::the_stop_event()
2. DisplayConfiguration::the_main_loop()
3. The factory is supplied at Platform creation time through mg::create_platform(MainLoopFactory) and events are created with a parameterless platform->create_bla_event()

I am fine with moving to the second option.

[Alan Griffiths (alan-griffiths)]

> > 361 + auto asio_event = std::static_pointer_cast<AsioMainLoopEvent>(event);
> > ...
> > 370 + auto asio_event = std::static_pointer_cast<AsioMainLoopEvent>(event);
> >
> > These casts look wrong - there should not be undefined behaviour if a
> conforming parameter is supplied.
> >
> > I'm still trying to decide if the casts should be dynamic, if the function
> signature is wrong, or if some other
> > way to recover type information is appropriate (e.g. double dispatch).
>
> The rationale for this is that there should be a single factory in use
> providing compatible events (i.e. that can be safely downcasted) , but this is
> indeed a fragile assumption. I am happy to change this to a dynamic_cast (or
> we can discuss more about the other options).

I now think this relates to my concerns about roles. (see below)

> > Does this really do what it claims? (In run_mir() init() is called before
> invoking run() -
> > so it may not be the best > way to test the required behaviour.
>
> At the moment we don't have a reliable mechanism to detect when the server has
> actually started. However, we start queuing signal events from the moment we
> create the DisplayServer, not from the moment we start running it, so the code
> above will work fine. I considered adding a way to notify the caller that the
> display server has actually started, e.g., a display_server_started callback,
> but I didn't want to overload this MP.

OK

> > 482 + std::shared_ptr<mir::MainLoopFactory> main_loop_factory;
> > Why does display server need a dependency on MainLoopFactory? AFAICS it just
> needs a MainLoop.
> > Can't MainLoopFactory be subsumed into DefaultServerConfiguration?
>
> The Display server needs to create a stop event, which needs a handle to a
> main loop factory.

Why is that the role of the DisplayServer? AFAICS it has no need to know anything about StopEvent.

The equivalent registration was in run_mir() which seems a better place to me.

> Also, in the near future the Platform will need to be able to create a
> platform events (e.g. PauseResume), which again need the main loop factory.
> The main loop factory can be supplied either at Platform construction time, or
> at event creation time(platform->create_bla_event(factory)).
>
> So one option is:
>
> 1. DisplayConfiguration::the_main_loop_factory()
> 2. DisplayServer uses the main loop factory to create the stop event
> 3. DisplayServer passes the main loop factory into
> platform->create_bla_event(factory) to create platform events

If it is more often for creating events I don't think that MainLoopFactory is the best name.

But do the event objects even need to be visible outside the implementation? Can't the whole interface be:

void MainLoop::register_signal_handler(std::initializer_list<int> signals, std::function<void(int)> const& handler);

> Alternatively the main loop factory can be subsumed into the configuration:
>
> 1. DisplayConfiguration::the_stop_event()
> 2. DisplayConfiguration::the_main_loop()
> 3. The factory is supplied at Platform creation time through
> mg::create_platform(MainLoopFactory) and events are created with a
> parameterless platform->create_bla_event()
>
> I a...

[Alexandros Frantzis (afrantzis)]

> Why is that the role of the DisplayServer? AFAICS it has no need to know
> anything about StopEvent.
>
> The equivalent registration was in run_mir() which seems a better place to me.

> But do the event objects even need to be visible outside the implementation?
> Can't the whole interface be:
>
> void MainLoop::register_signal_handler(std::initializer_list<int> signals,
> std::function<void(int)> const& handler);

Joining pieces from the discussion so far, the proposal becomes (leaving out details
not related to this MP):

========================
class MainLoop
{
    void register_signal_handler(std::initializer_list<int> signals,
                                 std::function<void(int)> const& handler) = 0;

    /* In the future */
    void register_fd_handler(...) = 0;
};

std::shared_ptr<MainLoop> DisplayConfiguration::the_main_loop();

mir::run_mir(DisplayConfiguration& conf)
{
    DisplayServer server{conf}
    conf.the_main_loop()->register_signal_handler({SIGINT, SIGTERM}, [&server](int){server.stop()});
    ....
}

... somewhere in display_server.cpp:

// main_loop = conf.the_main_loop();
platform->register_pause_resume_handler(main_loop, handler);
=================================

I like this.

[Alan Griffiths (alan-griffiths)]

> > Why is that the role of the DisplayServer? AFAICS it has no need to know
> > anything about StopEvent.
> >
> > The equivalent registration was in run_mir() which seems a better place to
> me.
>
> > But do the event objects even need to be visible outside the implementation?
> > Can't the whole interface be:
> >
> > void MainLoop::register_signal_handler(std::initializer_list<int> signals,
> > std::function<void(int)> const& handler);
>
> Joining pieces from the discussion so far, the proposal becomes (leaving out
> details
> not related to this MP):
>
> ========================
> class MainLoop
> {
> void register_signal_handler(std::initializer_list<int> signals,
> std::function<void(int)> const& handler) = 0;
>
> /* In the future */
> void register_fd_handler(...) = 0;
> };
>
> std::shared_ptr<MainLoop> DisplayConfiguration::the_main_loop();
>
> mir::run_mir(DisplayConfiguration& conf)
> {
> DisplayServer server{conf}
> conf.the_main_loop()->register_signal_handler({SIGINT, SIGTERM},
> [&server](int){server.stop()});
> ....
> }
>
> ... somewhere in display_server.cpp:
>
> // main_loop = conf.the_main_loop();
> platform->register_pause_resume_handler(main_loop, handler);
> =================================
>
> I like this.

+1

BTW Is it worth also providing a forwarding wrapper for register_signal_handler() that gets rid of the explict creation of an initializer list in client code:

template<typename Signals...>
void register_signal_handler(std::function<void(int)> const& handler, Signals... signals)

[Alexandros Frantzis (afrantzis)]

> BTW Is it worth also providing a forwarding wrapper for register_signal_handler() that gets rid of the explict
> creation of an initializer list in client code:

> template<typename Signals...>
> void register_signal_handler(std::function<void(int)> const& handler, Signals... signals)

I left this for a future MP. I guess that in that case it would also make sense to reverse the arguments of the non-templated function for uniformity:

register_signal_handler(std::function<void(int)> const& handler, std::initializer_list<int> signals)

[Alan Griffiths (alan-griffiths)]

Much clearer. A couple of nits:

114 + virtual ~MainLoop() {}

Are we really expecting destructors to throw?

~~~~

456 +#include <iostream>

Should be unnecessary

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:572
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~afrantzis/mir/display-server-main-loop-1/+merge/157693/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/328/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/305
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/188
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/333
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/333/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/139

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

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> 114 + virtual ~MainLoop() {}
> Are we really expecting destructors to throw?

Fixed.

> 456 +#include <iostream>
> Should be unnecessary

Already fixed in 573, probably the diff didn't refresh quickly enough.

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:574
http://jenkins.qa.ubuntu.com/job/mir-ci/330/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/307
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/190
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/335
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/335/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/142

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== added file 'include/server/mir/asio_main_loop.h'
--- include/server/mir/asio_main_loop.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/asio_main_loop.h	2013-04-08 16:58:20 +0000
@@ -0,0 +1,53 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser 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 Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#ifndef MIR_ASIO_MAIN_LOOP_H_
+#define MIR_ASIO_MAIN_LOOP_H_
+
+#include "mir/main_loop.h"
+
+#include <boost/asio.hpp>
+#include <memory>
+#include <vector>
+
+namespace mir
+{
+
+class AsioMainLoop : public MainLoop
+{
+public:
+    AsioMainLoop();
+    ~AsioMainLoop() noexcept(true);
+
+    void run();
+    void stop();
+
+    void register_signal_handler(
+        std::initializer_list<int> signals,
+        std::function<void(int)> const& handler);
+
+private:
+    class SignalHandler;
+
+    boost::asio::io_service io;
+    std::vector<std::unique_ptr<SignalHandler>> signal_handlers;
+};
+
+}
+
+#endif /* MIR_ASIO_MAIN_LOOP_H */

=== modified file 'include/server/mir/default_server_configuration.h'
--- include/server/mir/default_server_configuration.h	2013-03-27 15:12:09 +0000
+++ include/server/mir/default_server_configuration.h	2013-04-08 16:58:20 +0000
@@ -123,6 +123,8 @@
     virtual std::shared_ptr<shell::SurfaceBuilder> the_surface_builder();
     virtual std::shared_ptr<time::TimeSource> the_time_source();
 
+    virtual std::shared_ptr<MainLoop> the_main_loop();
+
 protected:
     virtual std::shared_ptr<options::Option> the_options() const;
     virtual std::shared_ptr<input::InputChannelFactory> the_input_channel_factory();
@@ -149,6 +151,7 @@
     CachedPtr<graphics::DisplayReport> display_report;
     CachedPtr<surfaces::SurfaceController> surface_controller;
     CachedPtr<time::TimeSource> time_source;
+    CachedPtr<MainLoop> main_loop;
 
 private:
     std::shared_ptr<options::Option> options;

=== added file 'include/server/mir/main_loop.h'
--- include/server/mir/main_loop.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/main_loop.h	2013-04-08 16:58:20 +0000
@@ -0,0 +1,48 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser 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 Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#ifndef MIR_MAIN_LOOP_H_
+#define MIR_MAIN_LOOP_H_
+
+#include <functional>
+#include <initializer_list>
+
+namespace mir
+{
+
+class MainLoop
+{
+public:
+    virtual ~MainLoop() = default;
+
+    virtual void run() = 0;
+    virtual void stop() = 0;
+
+    virtual void register_signal_handler(
+        std::initializer_list<int> signals,
+        std::function<void(int)> const& handler) = 0;
+
+protected:
+    MainLoop() = default;
+    MainLoop(MainLoop const&) = delete;
+    MainLoop& operator=(MainLoop const&) = delete;
+};
+
+}
+
+#endif /* MIR_MAIN_LOOP_H_ */

=== modified file 'include/server/mir/server_configuration.h'
--- include/server/mir/server_configuration.h	2013-03-22 17:13:10 +0000
+++ include/server/mir/server_configuration.h	2013-04-08 16:58:20 +0000
@@ -41,6 +41,8 @@
 class EventFilter;
 }
 
+class MainLoop;
+
 class ServerConfiguration
 {
 public:
@@ -49,6 +51,7 @@
     virtual std::shared_ptr<graphics::Display> the_display() = 0;
     virtual std::shared_ptr<compositor::Compositor> the_compositor() = 0;
     virtual std::shared_ptr<input::InputManager> the_input_manager() = 0;
+    virtual std::shared_ptr<MainLoop> the_main_loop() = 0;
 
 protected:
     ServerConfiguration() = default;

=== removed file 'include/server/mir/signal_dispatcher.h'
--- include/server/mir/signal_dispatcher.h	2013-03-28 12:13:18 +0000
+++ include/server/mir/signal_dispatcher.h	1970-01-01 00:00:00 +0000
@@ -1,61 +0,0 @@
-/*
- * Copyright © 2012 Canonical Ltd.
- *
- * This program is free software: you can redistribute it and/or modify it
- * under the terms of the GNU Lesser 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 Lesser General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- *
- * Authored by: Thomas Voss <[e-mail]>
- */
-
-#ifndef MIR_SIGNAL_DISPATCHER_H_
-#define MIR_SIGNAL_DISPATCHER_H_
-
-#include <boost/signals2.hpp>
-
-#include <memory>
-
-namespace mir
-{
-
-// Singleton class that decouples
-// signal handling from reacting to
-// signals to ensure that the actual signal
-// handler only calls async signal-safe functions
-// according to man 7 signal.
-class SignalDispatcher
-{
-public:
-    struct Constructor;
-
-    typedef boost::signals2::signal<void(int)> SignalType;
-
-    static std::shared_ptr<SignalDispatcher> instance();
-
-    ~SignalDispatcher();
-
-    SignalType& signal_channel();
-
-    void enable_for(int signal);
-
-private:
-    friend struct Constructor;
-
-    SignalDispatcher();
-    SignalDispatcher(const SignalDispatcher&) = delete;
-    SignalDispatcher& operator=(const SignalDispatcher&) = delete;
-
-    struct Private;
-    std::unique_ptr<Private> p;
-};
-}
-
-#endif // MIR_SIGNAL_DISPATCHER_H_

=== modified file 'src/server/CMakeLists.txt'
--- src/server/CMakeLists.txt	2013-04-08 02:55:16 +0000
+++ src/server/CMakeLists.txt	2013-04-08 16:58:20 +0000
@@ -25,7 +25,7 @@
   run_mir.cpp
   display_server.cpp
   default_server_configuration.cpp
-  signal_dispatcher.cpp
+  asio_main_loop.cpp
 )
 
 set(MIRSERVER_LINKAGE SHARED)

=== added file 'src/server/asio_main_loop.cpp'
--- src/server/asio_main_loop.cpp	1970-01-01 00:00:00 +0000
+++ src/server/asio_main_loop.cpp	2013-04-08 16:58:20 +0000
@@ -0,0 +1,96 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser 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 Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "mir/asio_main_loop.h"
+
+#include <cassert>
+
+class mir::AsioMainLoop::SignalHandler
+{
+public:
+    SignalHandler(boost::asio::io_service& io,
+                  std::initializer_list<int> signals,
+                  std::function<void(int)> const& handler)
+        : signal_set{io},
+          handler{handler}
+    {
+        for (auto sig : signals)
+            signal_set.add(sig);
+    }
+
+    void async_wait()
+    {
+        signal_set.async_wait(
+            std::bind(&SignalHandler::handle, this,
+                      std::placeholders::_1, std::placeholders::_2));
+    }
+
+private:
+    void handle(boost::system::error_code err, int sig)
+    {
+        if (!err)
+        {
+            handler(sig);
+            signal_set.async_wait(
+                std::bind(&SignalHandler::handle, this,
+                          std::placeholders::_1, std::placeholders::_2));
+        }
+    }
+
+    boost::asio::signal_set signal_set;
+    std::function<void(int)> handler;
+};
+
+/* 
+ * We need to define an empty constructor and destructor in the .cpp file,
+ * so that we can use unique_ptr to hold SignalHandler. Otherwise, users
+ * of AsioMainLoop end up creating default constructors and destructors
+ * that don't have complete type information for SignalHandler and fail
+ * to compile.
+ */
+mir::AsioMainLoop::AsioMainLoop()
+{
+}
+
+mir::AsioMainLoop::~AsioMainLoop() noexcept(true)
+{
+}
+
+void mir::AsioMainLoop::run()
+{
+    io.run();
+}
+
+void mir::AsioMainLoop::stop()
+{
+    io.stop();
+}
+
+void mir::AsioMainLoop::register_signal_handler(
+    std::initializer_list<int> signals,
+    std::function<void(int)> const& handler)
+{
+    assert(handler);
+
+    auto sig_handler = std::unique_ptr<SignalHandler>{
+        new SignalHandler{io, signals, handler}};
+
+    sig_handler->async_wait();
+
+    signal_handlers.push_back(std::move(sig_handler));
+}

=== modified file 'src/server/default_server_configuration.cpp'
--- src/server/default_server_configuration.cpp	2013-04-02 20:56:09 +0000
+++ src/server/default_server_configuration.cpp	2013-04-08 16:58:20 +0000
@@ -18,6 +18,7 @@
 
 #include "mir/default_server_configuration.h"
 #include "mir/abnormal_exit.h"
+#include "mir/asio_main_loop.h"
 
 #include "mir/options/program_option.h"
 #include "mir/compositor/buffer_allocation_strategy.h"
@@ -515,3 +516,12 @@
             return std::make_shared<mir::time::HighResolutionClock>();
         });
 }
+
+std::shared_ptr<mir::MainLoop> mir::DefaultServerConfiguration::the_main_loop()
+{
+    return main_loop(
+        []()
+        {
+            return std::make_shared<mir::AsioMainLoop>();
+        });
+}

=== modified file 'src/server/display_server.cpp'
--- src/server/display_server.cpp	2013-03-21 03:32:59 +0000
+++ src/server/display_server.cpp	2013-04-08 16:58:20 +0000
@@ -20,6 +20,7 @@
 
 #include "mir/display_server.h"
 #include "mir/server_configuration.h"
+#include "mir/main_loop.h"
 
 #include "mir/compositor/compositor.h"
 #include "mir/frontend/shell.h"
@@ -27,11 +28,6 @@
 #include "mir/graphics/display.h"
 #include "mir/input/input_manager.h"
 
-#include <mutex>
-#include <thread>
-#include <mutex>
-#include <condition_variable>
-
 namespace mc = mir::compositor;
 namespace mf = mir::frontend;
 namespace mg = mir::graphics;
@@ -45,7 +41,7 @@
           shell{config.the_frontend_shell()},
           communicator{config.the_communicator()},
           input_manager{config.the_input_manager()},
-          exit(false)
+          main_loop{config.the_main_loop()}
     {
     }
 
@@ -54,9 +50,7 @@
     std::shared_ptr<frontend::Shell> shell;
     std::shared_ptr<mf::Communicator> communicator;
     std::shared_ptr<mi::InputManager> input_manager;
-    std::mutex exit_guard;
-    std::condition_variable exit_cv;
-    bool exit;
+    std::shared_ptr<mir::MainLoop> main_loop;
 };
 
 mir::DisplayServer::DisplayServer(ServerConfiguration& config) :
@@ -72,14 +66,11 @@
 
 void mir::DisplayServer::run()
 {
-    std::unique_lock<std::mutex> lk(p->exit_guard);
-
     p->communicator->start();
     p->compositor->start();
     p->input_manager->start();
 
-    while (!p->exit)
-        p->exit_cv.wait(lk);
+    p->main_loop->run();
 
     p->input_manager->stop();
     p->compositor->stop();
@@ -87,7 +78,5 @@
 
 void mir::DisplayServer::stop()
 {
-    std::unique_lock<std::mutex> lk(p->exit_guard);
-    p->exit = true;
-    p->exit_cv.notify_one();
+    p->main_loop->stop();
 }

=== modified file 'src/server/run_mir.cpp'
--- src/server/run_mir.cpp	2013-03-28 12:52:24 +0000
+++ src/server/run_mir.cpp	2013-04-08 16:58:20 +0000
@@ -18,34 +18,27 @@
 
 #include "mir/run_mir.h"
 #include "mir/display_server.h"
-#include "mir/signal_dispatcher.h"
+#include "mir/main_loop.h"
+#include "mir/server_configuration.h"
 
-#include <atomic>
-#include <thread>
 #include <csignal>
-
-namespace
-{
-std::atomic<mir::DisplayServer*> signal_display_server;
-
-void signal_terminate(int)
-{
-    while (!signal_display_server.load())
-        std::this_thread::yield();
-
-    signal_display_server.load()->stop();
-}
-}
+#include <cassert>
 
 void mir::run_mir(ServerConfiguration& config, std::function<void(DisplayServer&)> init)
 {
-    auto const sdp = SignalDispatcher::instance();
-    sdp->enable_for(SIGINT);
-    sdp->enable_for(SIGTERM);
-    sdp->signal_channel().connect(&signal_terminate);
+    DisplayServer* server_ptr{nullptr};
+    auto main_loop = config.the_main_loop();
+
+    main_loop->register_signal_handler(
+        {SIGINT, SIGTERM},
+        [&server_ptr](int)
+        {
+            assert(server_ptr);
+            server_ptr->stop();
+        });
 
     DisplayServer server(config);
-    signal_display_server.store(&server);
+    server_ptr = &server;
 
     init(server);
     server.run();

=== removed file 'src/server/signal_dispatcher.cpp'
--- src/server/signal_dispatcher.cpp	2013-04-02 15:43:52 +0000
+++ src/server/signal_dispatcher.cpp	1970-01-01 00:00:00 +0000
@@ -1,170 +0,0 @@
-/*
- * Copyright © 2012 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: Thomas Voss <[e-mail]>
- */
-
-#include "mir/signal_dispatcher.h"
-
-#include <mutex>
-#include <thread>
-#include <signal.h>
-#include <sys/types.h>
-#include <sys/socket.h>
-
-
-namespace
-{
-
-struct EventSocketPair
-{
-    typedef unsigned int SocketIdentifier;
-
-    static const SocketIdentifier socket_a = 0;
-    static const SocketIdentifier socket_b = 1;
-
-    EventSocketPair()
-    {
-        if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0)
-            throw std::runtime_error("Couldn't create socketpair");
-    }
-
-    ~EventSocketPair()
-    {
-        ::close(sockets[0]);
-        ::close(sockets[1]);
-    }
-
-    void write_signal_to(SocketIdentifier id, int signal)
-    {
-        if (::write(sockets[id], &signal, sizeof(signal)) < 0)
-        {
-            // TODO: We cannot throw an exception in code that is
-            // called from a signal handler. Reenable once we have
-            // signal-safe logging in place.
-            // throw std::runtime_error("Problem writing to socket");
-        }
-    }
-
-    int read_signal_from(SocketIdentifier id)
-    {
-        int signal{0};
-        ssize_t nread{0};
-
-        while (nread <= 0)
-        {
-            nread = ::read(sockets[id], &signal, sizeof(signal));
-
-            if (nread == 0)
-                throw std::runtime_error("Write end of socket has been closed");
-            else if (nread > 0 && nread < static_cast<ssize_t>(sizeof(signal)))
-                throw std::runtime_error("Incomplete message retrieved");
-            else if (nread < 0 && errno != EINTR)
-                throw std::runtime_error("Problem reading from socket");
-        }
-
-        return signal;
-    }
-
-    int sockets[2];
-} event_socket_pair;
-
-}
-
-struct mir::SignalDispatcher::Constructor
-{
-    static SignalDispatcher* construct()
-    {
-        return new SignalDispatcher();
-    }
-};
-
-namespace
-{
-std::shared_ptr<mir::SignalDispatcher> instance;
-std::once_flag init_flag;
-
-void init()
-{
-    instance.reset(mir::SignalDispatcher::Constructor::construct());
-}
-
-void signal_handler(int signal)
-{
-    event_socket_pair.write_signal_to(
-        EventSocketPair::socket_a,
-        signal);
-}
-
-}
-
-struct mir::SignalDispatcher::Private
-{
-    Private() : worker_thread(std::bind(&Private::worker, this))
-    {
-        worker_thread.detach();
-    }
-
-    void worker()
-    {
-        while(true)
-        {
-            // Todo: Use polling here.
-            try
-            {
-                int s = event_socket_pair.read_signal_from(
-                    EventSocketPair::socket_b);
-                signal_channel(s);
-            } catch(...)
-            {
-                break;
-            }
-        }
-    }
-
-    std::thread worker_thread;
-    mir::SignalDispatcher::SignalType signal_channel;
-};
-
-std::shared_ptr<mir::SignalDispatcher> mir::SignalDispatcher::instance()
-{
-    std::call_once(::init_flag, ::init);
-
-    return ::instance;
-}
-
-mir::SignalDispatcher::SignalDispatcher() : p(new Private())
-{
-}
-
-mir::SignalDispatcher::~SignalDispatcher()
-{
-}
-
-mir::SignalDispatcher::SignalType& mir::SignalDispatcher::signal_channel()
-{
-    return p->signal_channel;
-}
-
-void mir::SignalDispatcher::enable_for(int signal)
-{
-    struct sigaction action;
-    ::memset(&action, 0, sizeof(action));
-    ::sigemptyset(&action.sa_mask);
-    ::sigaddset(&action.sa_mask, signal);
-    action.sa_handler = ::signal_handler;
-    action.sa_flags = SA_RESTART | SA_NODEFER;
-    ::sigaction(signal, &action, NULL);
-}

=== modified file 'tests/integration-tests/CMakeLists.txt'
--- tests/integration-tests/CMakeLists.txt	2013-04-02 21:01:03 +0000
+++ tests/integration-tests/CMakeLists.txt	2013-04-08 16:58:20 +0000
@@ -5,6 +5,7 @@
   test_surfaceloop.cpp
   test_error_reporting.cpp
   test_display_info.cpp
+  test_display_server_main_loop_events.cpp
 )
 
 add_subdirectory(client/)

=== modified file 'tests/integration-tests/cucumber/test_session_management_context.cpp'
--- tests/integration-tests/cucumber/test_session_management_context.cpp	2013-03-29 16:51:35 +0000
+++ tests/integration-tests/cucumber/test_session_management_context.cpp	2013-04-08 16:58:20 +0000
@@ -52,6 +52,7 @@
     MOCK_METHOD0(the_input_manager, std::shared_ptr<mi::InputManager>());
     MOCK_METHOD0(the_display, std::shared_ptr<mg::Display>());
     MOCK_METHOD0(the_compositor, std::shared_ptr<mc::Compositor>());
+    MOCK_METHOD0(the_main_loop, std::shared_ptr<mir::MainLoop>());
 };
 
 MATCHER_P(NamedWindowWithNoGeometry, name, "")

=== modified file 'tests/integration-tests/process/CMakeLists.txt'
--- tests/integration-tests/process/CMakeLists.txt	2013-03-13 04:54:15 +0000
+++ tests/integration-tests/process/CMakeLists.txt	2013-04-08 16:58:20 +0000
@@ -1,6 +1,5 @@
 list(APPEND INTEGRATION_TESTS_SRCS
   ${CMAKE_CURRENT_SOURCE_DIR}/test_process.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/test_signal_dispatcher.cpp
 )
 
 set(

=== removed file 'tests/integration-tests/process/test_signal_dispatcher.cpp'
--- tests/integration-tests/process/test_signal_dispatcher.cpp	2013-03-28 12:13:18 +0000
+++ tests/integration-tests/process/test_signal_dispatcher.cpp	1970-01-01 00:00:00 +0000
@@ -1,121 +0,0 @@
-/*
- * Copyright © 2012 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: Thomas Voss <[e-mail]>
- */
-
-#include "mir/signal_dispatcher.h"
-#include "mir_test_framework/process.h"
-
-#include <chrono>
-#include <thread>
-#include <mutex>
-#include <condition_variable>
-#include <gtest/gtest.h>
-
-namespace mtf = mir_test_framework;
-
-struct SignalCollector
-{
-    SignalCollector() : signal(-1)
-    {
-    }
-
-    void operator()(int s)
-    {
-        std::unique_lock<std::mutex> lock(mutex);
-        signal = s;
-        cv.notify_all();
-    }
-
-    int load()
-    {
-        unsigned int counter = 0;
-        std::unique_lock<std::mutex> lock(mutex);
-        while (signal == -1 && counter < 100)
-        {
-            cv.wait_for(lock, std::chrono::milliseconds(10));
-            counter++;
-        }
-        return signal;
-    }
-
-    int signal;
-
-    std::mutex mutex;
-    std::condition_variable cv;
-
-    SignalCollector(SignalCollector const&) = delete;
-};
-
-void a_main_function_accessing_the_signal_dispatcher()
-{
-    // Ensure that the SignalDispatcher has been created.
-    mir::SignalDispatcher::instance();
-    // Don't return (and exit) before the parent process has a chance to signal
-    std::this_thread::sleep_for(std::chrono::milliseconds(10));
-}
-
-template<int signal>
-void a_main_function_collecting_received_signals()
-{
-    mir::SignalDispatcher::instance()->enable_for(signal);
-    SignalCollector sc;
-    boost::signals2::scoped_connection conn(
-        mir::SignalDispatcher::instance()->signal_channel().connect(boost::ref(sc)));
-
-    ::kill(getpid(), signal);
-
-    EXPECT_EQ(signal, sc.load());
-}
-
-int a_successful_exit_function()
-{
-    return EXIT_SUCCESS;
-}
-
-int a_gtest_exit_function()
-{
-    return ::testing::Test::HasFailure() ? EXIT_FAILURE : EXIT_SUCCESS;
-}
-
-
-TEST(SignalDispatcher,
-    DISABLED_a_default_dispatcher_does_not_catch_any_signals)
-{
-    auto p = mtf::fork_and_run_in_a_different_process(
-        a_main_function_accessing_the_signal_dispatcher,
-        a_successful_exit_function);
-
-    p->terminate();
-
-    EXPECT_TRUE(p->wait_for_termination().signalled());
-}
-
-TEST(SignalDispatcher,
-    DISABLED_enabling_a_signal_results_in_signal_channel_delivering_the_signal)
-{
-    auto p = mtf::fork_and_run_in_a_different_process(
-        a_main_function_collecting_received_signals<SIGTERM>,
-        a_successful_exit_function);
-
-    EXPECT_TRUE(p->wait_for_termination().succeeded());
-
-    p = mtf::fork_and_run_in_a_different_process(
-        a_main_function_collecting_received_signals<SIGINT>,
-        a_successful_exit_function);
-
-    EXPECT_TRUE(p->wait_for_termination().succeeded());
-}

=== added file 'tests/integration-tests/test_display_server_main_loop_events.cpp'
--- tests/integration-tests/test_display_server_main_loop_events.cpp	1970-01-01 00:00:00 +0000
+++ tests/integration-tests/test_display_server_main_loop_events.cpp	2013-04-08 16:58:20 +0000
@@ -0,0 +1,102 @@
+/*
+ * Copyright © 2013 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: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "mir/compositor/compositor.h"
+
+#include "mir_test_framework/testing_server_configuration.h"
+#include "mir_test_doubles/mock_input_manager.h"
+#include "mir/run_mir.h"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#include <sys/types.h>
+#include <unistd.h>
+
+namespace mi = mir::input;
+namespace mc = mir::compositor;
+namespace mtd = mir::test::doubles;
+namespace mtf = mir_test_framework;
+
+namespace
+{
+
+class MockCompositor : public mc::Compositor
+{
+public:
+    MOCK_METHOD0(start, void());
+    MOCK_METHOD0(stop, void());
+};
+
+class ServerConfig : public mtf::TestingServerConfiguration
+{
+public:
+    std::shared_ptr<mi::InputManager> the_input_manager() override
+    {
+        if (!mock_input_manager)
+        {
+            mock_input_manager = std::make_shared<mtd::MockInputManager>();
+
+            EXPECT_CALL(*mock_input_manager, start()).Times(1);
+            EXPECT_CALL(*mock_input_manager, stop()).Times(1);
+        }
+
+        return mock_input_manager;
+    }
+
+    std::shared_ptr<mc::Compositor> the_compositor() override
+    {
+        if (!mock_compositor)
+        {
+            mock_compositor = std::make_shared<MockCompositor>();
+
+            EXPECT_CALL(*mock_compositor, start()).Times(1);
+            EXPECT_CALL(*mock_compositor, stop()).Times(1);
+        }
+
+        return mock_compositor;
+    }
+
+private:
+    std::shared_ptr<mtd::MockInputManager> mock_input_manager;
+    std::shared_ptr<MockCompositor> mock_compositor;
+};
+
+}
+
+TEST(DisplayServerMainLoopEvents, display_server_shuts_down_properly_on_sigint)
+{
+    ServerConfig server_config;
+
+    mir::run_mir(server_config,
+                 [](mir::DisplayServer&)
+                 {
+                    kill(getpid(), SIGINT);
+                 });
+}
+
+TEST(DisplayServerMainLoopEvents, display_server_shuts_down_properly_on_sigterm)
+{
+    ServerConfig server_config;
+
+    mir::run_mir(server_config,
+                 [](mir::DisplayServer&)
+                 {
+                    kill(getpid(), SIGTERM);
+                 });
+}

=== modified file 'tests/unit-tests/CMakeLists.txt'
--- tests/unit-tests/CMakeLists.txt	2013-04-02 21:01:03 +0000
+++ tests/unit-tests/CMakeLists.txt	2013-04-08 16:58:20 +0000
@@ -1,7 +1,12 @@
 add_definitions(-DTEST_RECORDINGS_DIR="${CMAKE_CURRENT_SOURCE_DIR}/input_recordings/")
 include_directories(${DRM_INCLUDE_DIRS} ${GBM_INCLUDE_DIRS} ${CMAKE_SOURCE_DIR})
 
-set(UNIT_TEST_SOURCES test_gmock_fixes.cpp)
+set(
+  UNIT_TEST_SOURCES
+
+  test_gmock_fixes.cpp
+  test_asio_main_loop.cpp
+)
 
 add_subdirectory(options/)
 add_subdirectory(client/)

=== added file 'tests/unit-tests/test_asio_main_loop.cpp'
--- tests/unit-tests/test_asio_main_loop.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/test_asio_main_loop.cpp	2013-04-08 16:58:20 +0000
@@ -0,0 +1,144 @@
+/*
+ * Copyright © 2013 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: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "mir/asio_main_loop.h"
+
+#include <gtest/gtest.h>
+
+#include <thread>
+#include <atomic>
+
+#include <sys/types.h>
+#include <unistd.h>
+
+TEST(AsioMainLoopTest, signal_handled)
+{
+    int const signum{SIGUSR1};
+    int handled_signum{0};
+
+    mir::AsioMainLoop ml;
+
+    ml.register_signal_handler(
+        {signum},
+        [&handled_signum, &ml](int sig)
+        {
+           handled_signum = sig;
+           ml.stop();
+        });
+
+    kill(getpid(), signum);
+
+    ml.run();
+
+    ASSERT_EQ(signum, handled_signum);
+}
+
+
+TEST(AsioMainLoopTest, multiple_signals_handled)
+{
+    std::vector<int> const signals{SIGUSR1, SIGUSR2};
+    size_t const num_signals_to_send{10};
+    std::vector<int> handled_signals;
+    std::atomic<unsigned int> num_handled_signals{0};
+
+    mir::AsioMainLoop ml;
+
+    ml.register_signal_handler(
+        {signals[0], signals[1]},
+        [&handled_signals, &num_handled_signals](int sig)
+        {
+           handled_signals.push_back(sig);
+           ++num_handled_signals;
+        });
+                    
+
+    std::thread signal_sending_thread(
+        [&ml, num_signals_to_send, &signals, &num_handled_signals]
+        {
+            for (size_t i = 0; i < num_signals_to_send; i++)
+            {
+                kill(getpid(), signals[i % signals.size()]);
+                while (num_handled_signals <= i) std::this_thread::yield();
+            }
+            ml.stop();
+        });
+
+    ml.run();
+
+    signal_sending_thread.join();
+
+    ASSERT_EQ(num_signals_to_send, handled_signals.size());
+
+    for (size_t i = 0; i < num_signals_to_send; i++)
+        ASSERT_EQ(signals[i % signals.size()], handled_signals[i]) << " index " << i;
+}
+
+TEST(AsioMainLoopTest, all_registered_handlers_are_called)
+{
+    int const signum{SIGUSR1};
+    std::vector<int> handled_signum{0,0,0};
+
+    mir::AsioMainLoop ml;
+
+    ml.register_signal_handler(
+        {signum},
+        [&handled_signum, &ml](int sig)
+        {
+            handled_signum[0] = sig;
+            if (handled_signum[0] != 0 &&
+                handled_signum[1] != 0 &&
+                handled_signum[2] != 0)
+            {
+                ml.stop();
+            }
+        });
+
+    ml.register_signal_handler(
+        {signum},
+        [&handled_signum, &ml](int sig)
+        {
+            handled_signum[1] = sig;
+            if (handled_signum[0] != 0 &&
+                handled_signum[1] != 0 &&
+                handled_signum[2] != 0)
+            {
+                ml.stop();
+            }
+        });
+
+    ml.register_signal_handler(
+        {signum},
+        [&handled_signum, &ml](int sig)
+        {
+            handled_signum[2] = sig;
+            if (handled_signum[0] != 0 &&
+                handled_signum[1] != 0 &&
+                handled_signum[2] != 0)
+            {
+                ml.stop();
+            }
+        });
+
+    kill(getpid(), signum);
+
+    ml.run();
+
+    ASSERT_EQ(signum, handled_signum[0]);
+    ASSERT_EQ(signum, handled_signum[1]);
+    ASSERT_EQ(signum, handled_signum[2]);
+}