Mir client callbacks are not thread-safe

[Daniel van Vugt]

Mir clients may not be aware, but the callbacks made by libmirclient are not thread safe in any way.

An apparently simple single-threaded Mir client has 3 threads, which can execute concurrently:

1. The "main" thread, where the initial connection was made.
2. The socket thread, where the protocol is handled (mir_connected_callback, mir_surface_callback, MirSurfaceEvent)
3. The input thread, where input events are received (MirMotionEvent, MirKeyEvent)

Yes, you could receive a MirSurfaceEvent and MirMotionEvent simultaneously in different threads. Neither of which are the main thread.

This is how a Mir client works. Unfortunately the existence of these threads is not yet documented. I will update the docs. But this bug is really about finding a way to make the client API more thread safe for simple toolkits and clients that normally don't care about threads. UPDATE: Client API docs improved in r789.

To make matters worse, the client API functions are generally not thread safe either: mir_surface_* mir_connection_*. So libmirclient forces you to run in multiple threads and yet doesn't protect you at all against races and corruption that could arise from calling libmirclient from varying threads. UPDATE: Fixed in r827

[Daniel van Vugt (vanvugt)]

Another alternative solution would be to implement a mir_main_loop_wait() type function, which blocks and provides a mutual exclusion guarantee with input callbacks.

[Daniel van Vugt (vanvugt)]

If we had a kind of mir_main_loop_wait() function, which waits for events then that would be triply useful:
  1. We could use it to do event handling without callbacks, and with thread-safety guaranteed; and
  2. We could use it as a launching point to call the callback in the correct (main) thread; and
  3. We could eliminate the ugly sleep() calls in our demo clients.

[Robert Carr (robertcarr)]

I don't think this is bug. Toolkits have their own main loops and synchronization primitives. Which thread mirclient invokes the input handler callback on doesn't really have anything to do with which thread an application or toolkit ends up processing the event on.

I think 'Simple apps' should:
1. Use platform-api not libmirclient
2. Synchronize events themselves if required.

[Daniel van Vugt (vanvugt)]

I'm talking about existing apps written for existing toolkits. GTK for example.

It is a very important issue. If you allow multiple threads to run concurrently without any locking, operating on the same data, they will race. Hence bugs.

[Daniel van Vugt (vanvugt)]

Though the workaround is potentially simple:
Be aware that Mir is not thread safe and do your own locking in a toolkit port. It's a bit painful for apps and toolkits that don't have or yet need threading primitives though.

[Daniel van Vugt (vanvugt)]

OK, I agree it's not ideal to reproduce a mutex API in Mir. So I'll hold off on that.

I will certainly improve our docs to highlight the issue, so that fewer people will get a rude shock in future. And if I can think of a more elegant solution than replicating chunks of PThread in Mir then I'll propose it.

[kevin gunn (kgunn72)]

raising priority altho this will need to be a topic addressed via architecture followup sprint (tbd)

[Chris Gagnon (chris.gagnon)]

This is blocking automation in QA can this be given a higher priority?

[Daniel van Vugt (vanvugt)]

Linked an experimental client API enhancement which at least gets us half way there... working around the problem but not changing any existing logic: lp:~vanvugt/mir/event-queue

[kevin gunn (kgunn72)]

daniel & I discussed this bug specifically, going to see if we can devote some attention to this.

[Daniel van Vugt (vanvugt)]

As we already know, non-trivial toolkits can live with this issue. It's only native Mir apps and simple toolkits that need some help.

I have a solid plan in mind for solving this now, but we've lived with it so long it's clearly not critical.

[Daniel van Vugt (vanvugt)]

If I recall correctly, the aforementioned "plan" involved making the dequeuing of input events something the client API requests, rather than living in its own thread. I think I found this to be reasonably easy to do.

Don't remember anything else about the "solid plan" I had in July :(

[Daniel van Vugt (vanvugt)]

Dropped severity. Having been over two years now, we're obviously used to living with this.

[Alberto Aguirre (albaguirre)]

Umm whatever happened to the manual dispatch work?
https://code.launchpad.net/~raof/mir/manual-connection-dispatch/+merge/243613

[Michał Sawicz (saviq)]

Syncing task from Mir.