engine: event processing when using multithreading

Only one thread may handle events in the default context at any point
in time. If a second thread calls g_main_context_iteration() or
g_main_loop_run(), it blocks until the other thread releases ownership
of the context. In that case, the first thread may wake up because of
an event that the second thread waits for, in which case the second
thread may never wake up. See
https://mail.gnome.org/archives/gtk-list/2013-April/msg00040.html

This means that SyncEvolution can no longer rely on these functions
outside of the main thread. This affects Sleep() and the EDS backend.

As an interim solution, take over permanent ownership of the default
context in the main thread. This prevents fights over the ownership
when the main thread enters and leaves the main loop
repeatedly. Utility code using the main context must check for
ownership first and fall back to some other means when not the owner.

The assumption for the fallback is that the main thread will drive the
event loop, so polling with small delays for the expected status
change (like "view complete" in the EDS backend) is going to succeed
eventually.

A better solution would be to have one thread running the event loop
permanently and push all event handling into that thread. There is C++
utility code for such things in:
http://cxx-gtk-utils.sourceforge.net/2.0/index.html

See in particular the TaskManager class and its
make_task_when()/make_task_compose()/make_task_when_full() functions
for executing asynchronous results via a glib main loop, also the
Future::when() method and a number of other similar things in the
library.

4 files changed, 53 insertions, 23 deletions

diff --git a/src/backends/evolution/EvolutionSyncSource.h b/src/backends/evolution/EvolutionSyncSource.h
index ccdcf2ea..dfaa67c2 100644
--- a/src/backends/evolution/EvolutionSyncSource.h
+++ b/src/backends/evolution/EvolutionSyncSource.h
@@ -120,12 +120,19 @@ class EvolutionAsync {
     public:
     EvolutionAsync()
     {
-        m_loop = GMainLoopCXX(g_main_loop_new(NULL, FALSE), false);
+        m_loop = GMainLoopCXX(g_main_loop_new(NULL, TRUE), false);
     }
      
     /** start processing events */
     void run() {
-        g_main_loop_run(m_loop.get());
+        if (g_main_context_is_owner(g_main_context_default())) {
+            g_main_loop_run(m_loop.get());
+        } else {
+            // Let master thread handle events.
+            while (g_main_loop_is_running(m_loop.get())) {
+                Sleep(0.1);
+            }
+        }
     }
  
     /** stop processing events, to be called inside run() by callback */
diff --git a/src/syncevo/EDSClient.cpp b/src/syncevo/EDSClient.cpp
index 3077d9e7..ad140c87 100644
--- a/src/syncevo/EDSClient.cpp
+++ b/src/syncevo/EDSClient.cpp
@@ -77,7 +77,12 @@ ESourceRegistryCXX EDSRegistryLoader::sync()
         if (m_gerror) {
             m_gerror.throwError("creating source registry");
         }
-        g_main_context_iteration(NULL, true);
+        // Only master thread can drive the event processing.
+	if (g_main_context_is_owner(g_main_context_default())) {
+            g_main_context_iteration(NULL, true);
+        } else {
+            Sleep(0.1);
+        }
     }
 }
 
diff --git a/src/syncevo/SyncContext.cpp b/src/syncevo/SyncContext.cpp
index da8a066f..148f8c68 100644
--- a/src/syncevo/SyncContext.cpp
+++ b/src/syncevo/SyncContext.cpp
@@ -2901,6 +2901,13 @@ void SyncContext::initMain(const char *appname)
 
     // redirect glib logging into our own logging
     g_log_set_default_handler(Logger::glogFunc, NULL);
+
+    // Only the main thread may use the default GMainContext.
+    // Anything else is unsafe, see https://mail.gnome.org/archives/gtk-list/2013-April/msg00040.html
+    // util.cpp:Sleep() checks this and uses the default context
+    // when called by the main thread, otherwise falls back to
+    // select().
+    g_main_context_acquire(NULL);
 #endif
     if (atoi(getEnv("SYNCEVOLUTION_DEBUG", "0")) > 3) {
         SySync_ConsolePrintf = Logger::sysyncPrintf;
diff --git a/src/syncevo/util.cpp b/src/syncevo/util.cpp
index 63b12a27..b27a7999 100644
--- a/src/syncevo/util.cpp
+++ b/src/syncevo/util.cpp
@@ -764,31 +764,42 @@ double Sleep(double seconds)
     SuspendFlags &s = SuspendFlags::getSuspendFlags();
     if (s.getState() == SuspendFlags::NORMAL) {
 #ifdef HAVE_GLIB
-        bool triggered = false;
-        GLibEvent timeout(g_timeout_add(seconds * 1000,
-                                        SleepTimeout,
-                                        &triggered),
-                          "glib timeout");
-        while (!triggered) {
-            if (s.getState() != SuspendFlags::NORMAL) {
-                break;
+        // Only use glib if we are the owner of the main context.
+        // Otherwise we would interfere (?) with that owner or
+        // depend on it to drive the context (?). The glib docs
+        // don't say anything about this; in practice, it was
+        // observed that with some versions of glib, a second
+        // thread just blocked here when the main thread was not
+        // processing glib events.
+        if (g_main_context_is_owner(g_main_context_default())) {
+            bool triggered = false;
+            GLibEvent timeout(g_timeout_add(seconds * 1000,
+                                            SleepTimeout,
+                                            &triggered),
+                              "glib timeout");
+            while (!triggered) {
+                if (s.getState() != SuspendFlags::NORMAL) {
+                    break;
+                }
+                g_main_context_iteration(NULL, true);
             }
-            g_main_context_iteration(NULL, true);
-        }
-        // done
-        return 0;
-#else
-        // Only works when abort or suspend requests are delivered via signal.
-        // Not the case when used inside helper processes; but those have
-        // and depend on glib.
-        timeval delay;
-        delay.tv_sec = floor(seconds);
-        delay.tv_usec = (seconds - (double)delay.tv_sec) * 1e6;
-        if (select(0, NULL, NULL, NULL, &delay) != -1) {
             // done
             return 0;
         }
 #endif
+
+        // Fallback when glib is not available or unusable (= outside the main thread).
+        // Busy loop to detect abort requests.
+	Timespec deadline = start + Timespec(floor(seconds), (seconds - floor(seconds)) * 1e9);
+	while (deadline > Timespec::monotonic()) {
+            timeval delay;
+            delay.tv_sec = 0;
+            delay.tv_usec = 1e5;
+            select(0, NULL, NULL, NULL, &delay);
+            if (s.getState() != SuspendFlags::NORMAL) {
+                break;
+            }
+        }
     }
 
     // not done normally, calculate remaining time