[gvfs] MTP: Move handling of events to a thread pool

[Philip Langdale <philipl src gnome org>]

commit e625ffe4fbab40a1b2d25a67308c8dafe8e71212
Author: Philip Langdale <philipl overt org>
Date:   Sun Apr 10 10:44:46 2016 -0700

    MTP: Move handling of events to a thread pool
    
    In the next, and hopefully final, change in this series, I'm going
    to introduce a new way of checking for events that avoids the race
    condition which can lead to crashes on unmount in the current code.
    
    As part of that, event handling must move off the checking thread
    to a separate thread. But even the existing checking mechanism
    could stand to benefit from using a separate thread.
    
    It's generally undesirable for a thread that's polling for activity
    to potentially block for a long time, and this is what could happen
    today if there's a long transfer happening when an event comes in.
    
    With the separate handler thread, we can dispatch the work and then
    resume polling for activity quickly.
    
    I used a single-thread thread pool to handle the heavy lifting of
    queuing events - there's no benefit to writing that yourself.
    
    Finally, some thoughts on synchronization:
    
    1) The thread pool has the lifetime of the backend. It is important
    not to tie it to mount state, as this would lead to requiring holding
    the backend mutex to know it is safe to push an event to the pool,
    and that brings us back to potentially blocking the checking thread
    again.
    
    2) While the thread pool needs the extended lifetime, we don't want
    to handle events once unmount has started. We enforce this in two
    ways.
    
    a) We set the pool thread count to 0 in unmount. From here on,
    newly queued events will not get dispatched.
    
    b) We reorder the checks in handle_event() so that it always attempts
    to take the backend mutex first. After taking the mutex, it will
    check if an unmount is underway. This ensures that either an unmount
    has not started and it is safe to handle the event, or the unmount
    is complete, and we know we should drop the event.

 daemon/gvfsbackendmtp.c |   45 ++++++++++++++++++++++++++++++++++++++-------
 daemon/gvfsbackendmtp.h |    4 ++++
 2 files changed, 42 insertions(+), 7 deletions(-)
---
diff --git a/daemon/gvfsbackendmtp.c b/daemon/gvfsbackendmtp.c
index dbfe574..6fdc5f6 100644
--- a/daemon/gvfsbackendmtp.c
+++ b/daemon/gvfsbackendmtp.c
@@ -96,6 +96,13 @@ typedef struct {
   uint32_t id;
 } CacheEntry;
 
+#if HAVE_LIBMTP_1_1_5
+typedef struct {
+  LIBMTP_event_t event;
+  uint32_t param1;
+} EventData;
+#endif
+
 
 /************************************************
  * Static prototypes
@@ -106,6 +113,11 @@ emit_delete_event (gpointer key,
                    gpointer value,
                    gpointer user_data);
 
+#if HAVE_LIBMTP_1_1_5
+static void
+handle_event (EventData *data, GVfsBackendMtp *backend);
+#endif
+
 
 /************************************************
  * Storage name helper
@@ -364,6 +376,11 @@ g_vfs_backend_mtp_init (GVfsBackendMtp *backend)
 
   backend->monitors = g_hash_table_new (NULL, NULL);
 
+#if HAVE_LIBMTP_1_1_5
+  backend->event_pool = g_thread_pool_new ((GFunc) handle_event,
+                                           backend, 1, FALSE, NULL);
+#endif
+
   g_debug ("(I) g_vfs_backend_mtp_init done.\n");
 }
 
@@ -384,6 +401,10 @@ g_vfs_backend_mtp_finalize (GObject *object)
 
   backend = G_VFS_BACKEND_MTP (object);
 
+#if HAVE_LIBMTP_1_1_5
+  g_thread_pool_free (backend->event_pool, TRUE, TRUE);
+#endif
+
   g_hash_table_foreach (backend->monitors, remove_monitor_weak_ref, backend->monitors);
   g_hash_table_unref (backend->monitors);
 
@@ -574,8 +595,6 @@ on_uevent (GUdevClient *client, gchar *action, GUdevDevice *device, gpointer use
 }
 
 #if HAVE_LIBMTP_1_1_5
-static void handle_event (GVfsBackendMtp *backend, LIBMTP_event_t event, uint32_t param1);
-
 static gpointer
 check_event (gpointer user_data)
 {
@@ -603,8 +622,11 @@ check_event (gpointer user_data)
     }
 
     backend = g_weak_ref_get (event_ref);
-    if (backend) {
-      handle_event (backend, event, param1);
+    if (backend && !g_atomic_int_get (&backend->unmount_started)) {
+      EventData *ed = g_new (EventData, 1);
+      ed->event = event;
+      ed->param1 = param1;
+      g_thread_pool_push (backend->event_pool, ed, NULL);
       g_object_unref (backend);
     }
   }
@@ -612,10 +634,14 @@ check_event (gpointer user_data)
 }
 
 void
-handle_event (GVfsBackendMtp *backend, LIBMTP_event_t event, uint32_t param1)
+handle_event (EventData *ed, GVfsBackendMtp *backend)
 {
+  LIBMTP_event_t event = ed->event;
+  uint32_t param1 = ed->param1;
+  g_free (ed);
+
+  g_mutex_lock (&backend->mutex);
   if (!g_atomic_int_get (&backend->unmount_started)) {
-    g_mutex_lock (&backend->mutex);
     switch (event) {
     case LIBMTP_EVENT_STORE_ADDED:
       {
@@ -718,8 +744,8 @@ handle_event (GVfsBackendMtp *backend, LIBMTP_event_t event, uint32_t param1)
     default:
       break;
     }
-    g_mutex_unlock (&backend->mutex);
   }
+  g_mutex_unlock (&backend->mutex);
 }
 #endif
 
@@ -868,6 +894,11 @@ do_unmount (GVfsBackend *backend, GVfsJobUnmount *job,
 
   g_atomic_int_set (&op_backend->unmount_started, TRUE);
 
+#if HAVE_LIBMTP_1_1_5
+  /* It's no longer safe to handle events. */
+  g_thread_pool_set_max_threads (op_backend->event_pool, 0, NULL);
+#endif
+
   /* Emit delete events to tell clients files are gone. */
   GHashTableIter iter;
   gpointer key, value;
diff --git a/daemon/gvfsbackendmtp.h b/daemon/gvfsbackendmtp.h
index a62f326..0669f94 100644
--- a/daemon/gvfsbackendmtp.h
+++ b/daemon/gvfsbackendmtp.h
@@ -65,6 +65,10 @@ struct _GVfsBackendMtp
 
   gboolean android_extension;
   gboolean get_partial_object_capability;
+
+#ifdef HAVE_LIBMTP_1_1_5
+  GThreadPool *event_pool;
+#endif
 };
 
 struct _GVfsBackendMtpClass