Subject: Display server main control loop

From: Alexandros Frantzis <[email-masked]>
Date: Tue, 2 Apr 2013 15:40:45 +0300

Hi all,

while working on adding VT switching I noticed that the code started to
become more complicated, as I needed to handle asynchronous events (a
signal in this case) and coordinate cross-component operations.

One way to improve the situation is to add a main control loop to the
display server. This is *not* meant to be a loop for all kinds of events
(e.g.  input, graphics), only for external asynchronous events that need
to change the state of the main components of the display server in a
coordinated manner.

Such events are:

* Quitting (signals)
* VT switching (and more generally, display server pause/resume) (signals)
* Display reconfiguration (e.g. watching a udev FD on the desktop)

Although, we could conceivably handle these in a distributed manner, I
feel we would be adding a lot of complexity and fragility to the system.

There are a couple of mechanisms we could use to implement a main
control loop and that could handle events from all our sources (signals
and FDs for now).  We could use boost asio, or we could implement our
own mechanism, using signalfd to route the signals to an fd we can
select() on.

The main loop could live in the DisplayServer::run() method, replacing
the wait on the exit_cv condition variable that is currently there, or
it could run in a separate thread like SignalDispatcher currently does.
I prefer the first approach since the main display server thread is idle
anyway, and it's a natural place for such an event loop.

Thoughts? Any other events of this kind that we should be aware of?

Thanks,
Alexandros

---

From: Daniel van Vugt <[email-masked]>
Date: Wed, 03 Apr 2013 09:47:57 +0800

Yeah I agree any solution needs to replace the existing loop in 
DisplayServer::run(). We don't want two loops resembling a "main loop".

I'm still cautious about using the word "event" though. Maybe call these 
particular things "CoreEvent", or DisplayServer::Event's

---

From: Alexandros Frantzis <[email-masked]>
Date: Wed, 3 Apr 2013 18:51:38 +0300

> Thoughts? Any other events of this kind that we should be aware of?

To get a better feeling of what's involved, I created a prototype
abstraction for a main loop and an implementation using boost asio.
Get it and use it with:

$ bzr branch lp:~afrantzis/+junk/mainloop
$ cd mainloop && make
$ ./mainloop

The main loop abstraction was designed so that event creation and
handling are completely dissociated. The display server (or other
consumer) can handle an event without caring if it is coming from a
signal, a read from a fd, or another mechanism.  So, we could have
something like this in the mir main loop:

auto main_loop = main_loop_factory->create_main_loop();

auto pause_event = platform->create_pause_event(main_loop_factory);
auto disp_conf_event = platform->create_display_configuration_event(main_loop_factory);
auto quit_event = main_loop_factory->create_signal_event(SIGINT);

main_loop->register_event_handler(pause_event, pause_handler);
main_loop->register_event_handler(disp_conf_event, disp_conf_handler);
main_loop->register_event_handler(quit_event, [main_loop]{main_loop->stop();});

main_loop->run();

Thoughts?

Thanks,
Alexandros

---

From: Robert Ancell <[email-masked]>
Date: Thu, 04 Apr 2013 11:25:23 +1300

> main_loop->register_event_handler(quit_event, [main_loop]{main_loop->stop();});
I like having the event objects as opposed to just having these as
parameters when registering.

However it would seem to me more logical to register the handlers on the
event object and not the main loop. That would allow parameters on the
event handler functions (I'm not sure void() is going to be sufficient
for all events). I can't think of a good case to be able to use the same
handler for different event types.

I'm assuming the FD event is unfinished? It reads a requested amount of
bytes from a FD then calls the handler without giving access to the data?

---

From: Thomas Voß <[email-masked]>
Date: Thu, 04 Apr 2013 08:32:05 +0200

> something like this in the mir main loop:

This looks pretty good to me, some thoughts on the separation of
> auto pause_event = platform->create_pause_event(main_loop_factory);

I would rather prefer:

PauseEvent pause_event(main_loop);
pause_event.async_wait(pause_handler);

QuitEvent quit_event(main_loop);
quit_event.async_wait(quit_handler);

DisplayConfigurationEvent disp_config_event(main_loop);
disp_config_event.async_wait(disp_conf_handler);

See boost::asio::signal_set as an example: Different parties interested
in a certain set of signals just setup their respective signal set and
execute an (async) wait on the respective set. Behind the scenes, the
signal_set class sets up all of the handlers, and associates all of the
operation with one io_service, which forces handler execution to the
thread that the io_service.run method is executed on.

To this end, a mir::EventService would need to be created, and the
respective events, including custom handler types (not only void()), are
known to the EventService. On startup, the service is registered with
the io_service representing the main event loop with
boost::asio::add_service<mir::EventService>(io_service).

A class QuitEvent would then do:

class QuitEvent
{
public:
	QuitEvent(const std::shared_ptr<mir::MainLoop>& main_loop) :
io_service(main_loop->io_service()) {}

	void async_wait(QuitEventHandler handler)
	{
	
boost::asio::use_service<mir::EventService>(io_service).enqueue_handler_for_quit_event(handler);
	}
private:
	boost::asio::io_service& io_service;
};

>From my perspective, keeping the handler definition local, without
encoding everything within the main loop avoids tight coupling, and the
main loop is stripped down to only the synchronization concern.

Makes sense?

Thanks,

  Thomas

---

From: Alexandros Frantzis <[email-masked]>
Date: Thu, 4 Apr 2013 10:56:48 +0300

> bytes from a FD then calls the handler without giving access to the data?

The idea (in this prototype) is that main loop events do not carry
additional data. A particular amount of data being available on an FD (a
message) is just a notification that something interesting has happened,
in the same way a signal is; the contents of the message don't matter.
Data is being read from the FD to ensure we don't keep getting
notifications about the same message. This scheme is somewhat limited,
but does cover the use cases we currently care about.

Perhaps we can build more elaborate events (with custom handlers), on
top of this scheme. I will give this some more thought.

---

From: Alexandros Frantzis <[email-masked]>
Date: Thu, 4 Apr 2013 11:41:11 +0300

> disp_config_event.async_wait(disp_conf_handler);

The problem is that the way many events are triggered is platform
dependent. For example, a display configuration event may be triggered
by data being available on an FD on the desktop (from udev), and
something completely different on Android.  That's the reason I opted
for asking components to give us event object, since they have all the
information to set this up.

In the scheme you propose, would different triggers be handled by having
a different version of EventService for each platform?

> main loop is stripped down to only the synchronization concern.

What you are proposing is more versatile, but I see tighter coupling as
something desirable in this case. The main control loop is not supposed
to be a generic event loop, it's only about events that need to change
the state of the main components of the display server in a coordinated
manner. As such, the handlers need to defined at a level where all the
information is available, and that is at the control loop level. I don't
want to allow event handler registration away from the main loop, since
this is not useful for our use cases, and will make the code more
difficult to reason about.

The model I am proposing is that components offer interesting events
which are independent of any particular main loop, and at the main
control loop level we decide if/how to handle them. So, e.g., the
platforms tells us: "I am producing display configuration events, use
this event object if you want to handle them".

I would like to support events that provide more information (i.e. have
a custom handler) and I am currently thinking about how to go about
this, while still elegantly allowing different triggers depending on the
platform.

Thanks,
Alexandros

---

From: Thomas Voß <[email-masked]>
Date: Thu, 04 Apr 2013 11:25:19 +0200

> information to set this up.

Okay, the idea here is: "Client" code expresses an interest in a certain
event, say "DisplayReconfigurationEvent" and it's not a concern of the
code expressing interest in the event to know where this event might
originate from. That is, I would expect the respective Event classes
itself to know how to set themselves up and talk to the
platform-specific components. With that, we would achieve a strong
separation of concerns, while keeping platform-specific tasks local to
the Event classes.

>From my perspective, there is a difference between the concern of
synchronization and the concern of communicating specific events to
components interested in them. In this respect, I do agree with your
intention to avoid a generic event loop. What I'm proposing is
basically: We have a generic reactor (boost::asio::io_service) that is
concerned with synchronization and we can add specific services to
specific instances of the reactor, for example a
mir::display::ConfigurationEventService.

> this, while still elegantly allowing different triggers depending on the

I think my proposal addresses your requirement quite elegantly in that
the Event carries the platform-independent information but knows
internally how to wire itself up to the platform-specific pieces.
Components interested in the event then are able to say: I'm interested
in this event and would like to wait for it, not knowing anything about
something platform-specific.

---

From: Alexandros Frantzis <[email-masked]>
Date: Thu, 4 Apr 2013 18:26:04 +0300

> separation of concerns, while keeping platform-specific tasks local to

I find that we are disconnecting related entities too much this way. If
a component is responsible for producing an event, it should be the one
giving out event handles. Having "free" event objects essentially means
hiding a dependency (between the producer of the event and the event
object) that should be explicit.

Also, with the "free" objects it's still not clear to me how we will
handle different implementations on different platforms. Would this be
handled at build time (if PLATFORM = "android" build
pause_event_android.cpp else build pause_event_gbm.cpp) ?

> mir::display::ConfigurationEventService.

I am not convinced that using this model would be an win in our case.
As noted before, it's versatile, but at the same time more indirect and
complicated that we need (IMO).

Furthermore, if we go for this approach, we are going to build too tightly
around the boost asio model, whereas I am going for a more abstracted
approach. If decide at some point to change to another main loop
mechanism, we will need to change all of our event classes. With the
approach in the branch (and the latest changes, see below) custom events
use the lower-level MainLoopFactory services, which can be changed
easily at any time.

> something platform-specific.

I have updated lp:~afrantzis/+junk/mainloop to support custom events
(i.e. events with extra data, see TextEvent in component.*). The idea is
to build on the lower-level infrastructure offered by the
MainLoopFactory (signal and fd events) layer to provide custom events.

If we use custom events for everything then the lower-level event
infrastructure is not used at all in the main loop, and the code becomes
something like:

StopEvent stop_event{main_loop_factory}; // StopEvent is local to the main loop
DispConfEvent disp_event = platform->create_disp_conf_event(main_loop_factory);
PauseResumeEvent pause_resume_event = platform->create_pause_resume_event(main_loop_factory);

stop_event.register_handler(main_loop, [main_loop]() { main_loop->stop(); });
disp_event->register_handler(main_loop, []() { ... });
pause_resume_event->register_handler(main_loop, [...](bool pause) { if (pause) ... else ... });

main_loop->run();

Thanks,
Alexandros

---

From: Thomas Voß <[email-masked]>
Date: Fri, 05 Apr 2013 08:53:19 +0200

>>> information to set this up.

I think we are saying the same here :) So I'm good with your proposal.

>>> this, while still elegantly allowing different triggers depending on the

Fair enough, I'm mostly convinced :)

> PauseResumeEvent pause_resume_event = platform->create_pause_resume_event(main_loop_factory);

Fine with me, one niggle, though: Both DisplayConfigurationEvent and
PauseResumeEvent are constructed in a factory-style, whereas StopEvent
is not. Do you think it makes sense to unify the construction pattern here?

Thanks,

  Thomas

---

From: Alexandros Frantzis <[email-masked]>
Date: Fri, 5 Apr 2013 20:53:23 +0300

> is not. Do you think it makes sense to unify the construction pattern here?

Perhaps. It depends on whether we need to have platform specific stop
events (i.e. something else than SIGINT/SIGTERM). For now (in MP [1]), I
left it as a local class, but I am happy to revisit if our needs change.

Thanks,
Alexandros

[1] https://code.launchpad.net/~afrantzis/mir/display-server-main-loop/+merge/157447