[gimp] app: wait child processes with g_child_watch_add_full() rather than…

[Jehan <jehanp src gnome org>]

commit 2211ca7fd3a48b6d6d1e3deb2cd5cf096dcc07ad
Author: Jehan <jehan girinstud io>
Date:   Thu Apr 29 01:52:59 2021 +0200

    app: wait child processes with g_child_watch_add_full() rather than…
    
    … waitpid().
    
    My use case was when testing gimp_brush_select_new(). When doing tests,
    if I let the brush dialog opened while letting the plug-in exit cleanly
    otherwise, it somehow locked the process (I have stared so long at the
    code and still don't understand why).
    gimp_plug_in_close() would wait for it forever and the only way out was
    to kill GIMP completely. I guess we could try to run waitpid() with
    WNOHANG (and finally force-kill the child if it really won't exit) but
    it made the whole logics extra complicated.
    
    The logics with g_child_watch_add_full() is that we don't stop waiting
    for the child and just set a callback to finalize what needs to be. Now
    the worst case scenario would be to leave zombie processes dangling
    around, but it's better than freezing GIMP.
    
    Finally as a weird side effect, doing this change even unblocked the
    process with an unfinished brush selector so we don't even have a zombie
    anymore (at least for this specific case). All good in the end!
    
    Last side effect: it can speed up a tiny bit the plug-in close as we
    don't wait for processes anymore, which could be more visible at first
    startups (when we reload all the plug-ins). Though there is nothing
    scientific to my numbers, after multiple "first startups", it seems it
    went down from about 3.5 to 3.2 seconds on my specific machine. Nothing
    extra fancy, but we take what we can (and speeding up the startup was
    never the goal of this change anyway). It doesn't affect Windows which
    has its own logics to handle process termination and I preferred not to
    touch it.

 app/plug-in/gimpplugin.c | 59 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 53 insertions(+), 6 deletions(-)
---
diff --git a/app/plug-in/gimpplugin.c b/app/plug-in/gimpplugin.c
index d117812ad6..82514fe34c 100644
--- a/app/plug-in/gimpplugin.c
+++ b/app/plug-in/gimpplugin.c
@@ -111,6 +111,11 @@ static gboolean   gimp_plug_in_flush         (GIOChannel   *channel,
 static void       gimp_plug_in_set_dll_directory (const gchar *path);
 #endif
 
+#ifndef G_OS_WIN32
+static void       gimp_plug_in_close_waitpid     (GPid         pid,
+                                                  gint         status,
+                                                  GimpPlugIn  *plug_in);
+#endif
 
 
 G_DEFINE_TYPE (GimpPlugIn, gimp_plug_in, GIMP_TYPE_OBJECT)
@@ -370,6 +375,27 @@ out:
 }
 #endif
 
+#ifndef G_OS_WIN32
+static void
+gimp_plug_in_close_waitpid (GPid        pid,
+                            gint        status,
+                            GimpPlugIn *plug_in)
+{
+  GError *error = NULL;
+
+  if (plug_in->manager->gimp->be_verbose &&
+      ! g_spawn_check_exit_status (status, &error))
+    {
+      g_printerr ("Process for plug-in '%s' terminated with error: %s\n",
+                  gimp_object_get_name (plug_in),
+                  error->message);
+    }
+  g_clear_error (&error);
+
+  g_spawn_close_pid (pid);
+}
+#endif
+
 
 /*  public functions  */
 
@@ -664,12 +690,33 @@ gimp_plug_in_close (GimpPlugIn *plug_in,
             status = kill (- plug_in->pid, SIGKILL);
           else
             status = kill (plug_in->pid, SIGKILL);
-        }
 
-      /* Wait for the process to exit. This will happen
-       * immediately if it was just killed.
-       */
-      waitpid (plug_in->pid, &status, 0);
+          /* Wait for the process to exit. This will happen
+           * immediately as it was just killed.
+           */
+          waitpid (plug_in->pid, &status, 0);
+        }
+      else
+        {
+          /*
+           * If we don't kill it, it means the plug-in ended normally
+           * hence its process should just return without problem. And
+           * this is the case nearly all the time. Yet I had this one
+           * case where the process would never return (even though the
+           * plug-in returned with a result and no errors) and a
+           * waitpid() would block the main process forever.
+           * Thus use instead a child watch source. My idea was that in
+           * the rare case when the child process doesn't return, it is
+           * better to leave a zombie than freeze the core. As it turns
+           * out, doing this even made the broken process exit (does
+           * g_child_watch_add_full() do something better than
+           * waitpid()?).
+           */
+          g_object_ref (plug_in);
+          g_child_watch_add_full (G_PRIORITY_LOW, plug_in->pid,
+                                  (GChildWatchFunc) gimp_plug_in_close_waitpid,
+                                  plug_in, (GDestroyNotify) g_object_unref);
+        }
 
 #else /* G_OS_WIN32 */
 
@@ -700,9 +747,9 @@ gimp_plug_in_close (GimpPlugIn *plug_in,
             }
         }
 
+      g_spawn_close_pid (plug_in->pid);
 #endif /* G_OS_WIN32 */
 
-      g_spawn_close_pid (plug_in->pid);
       plug_in->pid = 0;
     }