[glib/wip/gsubprocess] GSubprocess: Add request_exit() and terminate() API

[Colin Walters <walters src gnome org>]

commit 8856591fbfc4c41581ab7a0ac5a1acf6133a682b
Author: Colin Walters <walters verbum org>
Date:   Mon May 21 17:12:55 2012 -0400

    GSubprocess: Add request_exit() and terminate() API
    
    This is now race-free when we use waitid().
    
    https://bugzilla.gnome.org/show_bug.cgi?id=672102

 docs/reference/gio/gio-sections.txt |    2 +
 gio/gio.symbols                     |    2 +
 gio/gsubprocess.c                   |  139 ++++++++++++++++++++++-------------
 gio/gsubprocess.h                   |    6 ++
 gio/tests/gsubprocess-testprog.c    |   12 +++
 gio/tests/gsubprocess.c             |   52 +++++++++++++
 6 files changed, 163 insertions(+), 50 deletions(-)
---
diff --git a/docs/reference/gio/gio-sections.txt b/docs/reference/gio/gio-sections.txt
index 73f96ca..e0484d7 100644
--- a/docs/reference/gio/gio-sections.txt
+++ b/docs/reference/gio/gio-sections.txt
@@ -3934,6 +3934,8 @@ g_subprocess_add_watch
 g_subprocess_add_watch_full
 g_subprocess_run_sync
 g_subprocess_wait_sync
+g_subprocess_request_exit
+g_subprocess_terminate
 <SUBSECTION Status>
 g_subprocess_get_status_code
 g_subprocess_get_pid
diff --git a/gio/gio.symbols b/gio/gio.symbols
index 64b4050..4331aec 100644
--- a/gio/gio.symbols
+++ b/gio/gio.symbols
@@ -1088,6 +1088,7 @@ g_subprocess_new
 g_subprocess_new_with_args
 g_subprocess_set_environment
 g_subprocess_query_success
+g_subprocess_request_exit
 g_subprocess_run_sync
 g_subprocess_run_sync_get_output_bytes
 g_subprocess_run_sync_get_stdout_utf8
@@ -1113,6 +1114,7 @@ g_subprocess_set_standard_output_unix_fd
 g_subprocess_set_working_directory
 g_subprocess_start
 g_subprocess_start_with_pipes
+g_subprocess_terminate
 g_subprocess_unsetenv
 g_subprocess_wait_sync
 #ifndef G_OS_WIN32
diff --git a/gio/gsubprocess.c b/gio/gsubprocess.c
index ab71691..faeab1f 100644
--- a/gio/gsubprocess.c
+++ b/gio/gsubprocess.c
@@ -27,6 +27,7 @@
 #include "giostream.h"
 #include "gmemoryinputstream.h"
 #include "glibintl.h"
+#include "glib-private.h"
 
 #include <string.h>
 #ifdef G_OS_UNIX
@@ -39,8 +40,8 @@
 
 typedef struct _GSubprocessClass GSubprocessClass;
 
-static gpointer
-g_subprocess_waitpid_helper (gpointer data);
+static void
+g_subprocess_unix_queue_waitpid (GSubprocess  *self);
 
 struct _GSubprocessClass {
   GObjectClass parent_class;
@@ -137,16 +138,15 @@ g_subprocess_finalize (GObject *object)
 {
   GSubprocess *self = G_SUBPROCESS (object);
 
-  if (self->state == G_SUBPROCESS_STATE_RUNNING
+  if (self->state > G_SUBPROCESS_STATE_BUILDING
       && !self->detached)
     {
 #ifdef G_OS_UNIX
-      GThread *reaper_thread;
-
-      reaper_thread = g_thread_new ("GSubprocess wait",
-				    g_subprocess_waitpid_helper,
-				    GINT_TO_POINTER (self->pid));
-      g_thread_unref (reaper_thread);
+      /* Here we need to actually call waitpid() to clean up the
+       * zombie.  In case the child hasn't actually exited, defer this
+       * cleanup to the worker thread.
+       */
+      g_subprocess_unix_queue_waitpid (self);
 #endif
       g_spawn_close_pid (self->pid);
     }
@@ -1151,27 +1151,23 @@ g_subprocess_run_sync (GSubprocess   *self,
 
 #ifdef G_OS_UNIX
 
-/*
- * This function is called in the case where the caller didn't
- * explicitly use _set_detached() or watch the child explicitly via
- * g_subprocess_add_watch().
- *
- * We don't want to block the calling thread in waitpid(), so
- * move it to a thread.
- *
- * For more efficiency we could move this to the GLib worker thread,
- * but callers should really know to use one of _set_detached() or
- * _add_watch().
- */
-static gpointer
-g_subprocess_waitpid_helper (gpointer data)
+static gboolean
+g_subprocess_unix_waitpid_dummy (gpointer data)
 {
-  GPid pid = GPOINTER_TO_INT (data);
-  gint status;
-
-  waitpid (pid, &status, 0);
+  return FALSE;
+}
 
-  return NULL;
+static void
+g_subprocess_unix_queue_waitpid (GSubprocess  *self)
+{
+  GMainContext *worker_context;
+  GSource *waitpid_source;
+  
+  worker_context = GLIB_PRIVATE_CALL (g_get_worker_context) ();
+  waitpid_source = g_child_watch_source_new (self->pid); 
+  g_source_set_callback (waitpid_source, g_subprocess_unix_waitpid_dummy, NULL, NULL);
+  g_source_attach (waitpid_source, worker_context);
+  g_source_unref (waitpid_source);
 }
 
 static inline void
@@ -1331,8 +1327,6 @@ g_subprocess_start_with_pipes (GSubprocess       *self,
 			  && self->stderr_to_stdout == FALSE,
 			  FALSE);
 
-  self->state = G_SUBPROCESS_STATE_RUNNING;
-  
 #ifdef G_OS_UNIX
   if (self->stdin_path)
     {
@@ -1445,6 +1439,7 @@ g_subprocess_start_with_pipes (GSubprocess       *self,
     goto out;
 
   ret = TRUE;
+  self->state = G_SUBPROCESS_STATE_RUNNING;
 
   if (stdin_pipe_fd != -1)
     {
@@ -1495,18 +1490,10 @@ g_subprocess_start_with_pipes (GSubprocess       *self,
  * g_subprocess_get_pid:
  * @self: a #GSubprocess
  *
- * Returns the identifier for this child process; this function may
- * not be used if g_subprocess_set_detached() has been called.
- *
- * <warning>Due to the way GLib presently implements subprocess
- * monitoring on Unix, attempting to use e.g. kill() to send a signal
- * to the child process has a race condition where the child process
- * may not exist.</warning>
- *
- * This function can only be called when the process is currently
- * running.  In particular, do not call this function after a callback
- * from a child watch such as g_subprocess_add_watch() has been
- * invoked.
+ * Returns the identifier for this child process. 
+
+ * This function may not be used if g_subprocess_set_detached() has
+ * been called.
  *
  * Since: 2.34
  */
@@ -1514,8 +1501,8 @@ GPid
 g_subprocess_get_pid (GSubprocess     *self)
 {
   g_return_val_if_fail (G_IS_SUBPROCESS (self), 0);
-  g_return_val_if_fail (self->state == G_SUBPROCESS_STATE_RUNNING, 0);
   g_return_val_if_fail (!self->detached, 0);
+  g_return_val_if_fail (self->state > G_SUBPROCESS_STATE_BUILDING, 0);
   
   return self->pid;
 }
@@ -1560,12 +1547,7 @@ g_subprocess_child_watch_func (GPid       pid,
   GSubprocessWatchTrampolineData *data = user_data;
 
   data->self->status_code = status_code;
-  if (data->self->state != G_SUBPROCESS_STATE_TERMINATED)
-    {
-      g_assert (data->self->state == G_SUBPROCESS_STATE_RUNNING);
-      g_spawn_close_pid (data->self->pid);
-      data->self->state = G_SUBPROCESS_STATE_TERMINATED;
-    }
+  data->self->state = G_SUBPROCESS_STATE_TERMINATED;
   
   if (data->callback)
     data->callback (data->self, data->user_data);
@@ -1653,7 +1635,7 @@ g_subprocess_create_source (GSubprocess             *self,
   g_return_val_if_fail (self->state == G_SUBPROCESS_STATE_RUNNING, 0);
   g_return_val_if_fail (!self->detached, 0);
 
-  source = g_child_watch_source_new (self->pid);
+  source = GLIB_PRIVATE_CALL (g_child_watch_source_new_with_flags) (self->pid, _G_CHILD_WATCH_FLAGS_WNOWAIT);
   g_source_set_priority (source, priority);
   trampoline_data = g_new (GSubprocessWatchTrampolineData, 1);
   trampoline_data->self = g_object_ref (self);
@@ -1854,6 +1836,63 @@ g_subprocess_wait_sync (GSubprocess        *self,
   return ret;
 }
 
+/**
+ * g_subprocess_request_exit:
+ * @self: a #GSubprocess
+ *
+ * Use an operating-system specific method to request a graceful exit
+ * of the process.  On Unix for example, this sends %SIGTERM.
+ *
+ * You can use g_subprocess_add_watch() to monitor the status of the
+ * process after calling this function.
+ *
+ * This function may not be used if g_subprocess_set_detached() has
+ * been called.
+ */
+void
+g_subprocess_request_exit (GSubprocess       *self)
+{
+  g_return_if_fail (G_IS_SUBPROCESS (self));
+  g_return_if_fail (!self->detached);
+  g_return_if_fail (self->state > G_SUBPROCESS_STATE_BUILDING);
+
+  if (self->state == G_SUBPROCESS_STATE_TERMINATED)
+    return;
+
+#ifdef G_OS_UNIX
+  (void) kill (self->pid, SIGTERM);
+#endif
+}
+
+/**
+ * g_subprocess_terminate:
+ * @self: a #GSubprocess
+ *
+ * Use an operating-system specific method to attempt a forceful
+ * termination of the process.  On Unix for example, this sends
+ * %SIGKILL.
+ *
+ * You can use g_subprocess_add_watch() to monitor the status of the
+ * process after calling this function.
+ *
+ * This function may not be used if g_subprocess_set_detached() has
+ * been called.
+ */
+void             
+g_subprocess_terminate (GSubprocess       *self)
+{
+  g_return_if_fail (G_IS_SUBPROCESS (self));
+  g_return_if_fail (!self->detached);
+  g_return_if_fail (self->state > G_SUBPROCESS_STATE_BUILDING);
+
+  if (self->state == G_SUBPROCESS_STATE_TERMINATED)
+    return;
+
+#ifdef G_OS_UNIX
+  (void) kill (self->pid, SIGKILL);
+#endif
+}
+
 /**** High level wrapers ****/
 
 typedef struct {
diff --git a/gio/gsubprocess.h b/gio/gsubprocess.h
index 72200e0..15380d6 100644
--- a/gio/gsubprocess.h
+++ b/gio/gsubprocess.h
@@ -225,6 +225,12 @@ gboolean         g_subprocess_wait_sync (GSubprocess   *self,
 					 GCancellable  *cancellable,
 					 GError       **error);
 
+GLIB_AVAILABLE_IN_2_34
+void             g_subprocess_request_exit (GSubprocess       *self);
+
+GLIB_AVAILABLE_IN_2_34
+void             g_subprocess_terminate (GSubprocess       *self);
+
 /**** High level wrapers ****/
 
 GLIB_AVAILABLE_IN_2_34
diff --git a/gio/tests/gsubprocess-testprog.c b/gio/tests/gsubprocess-testprog.c
index 63f5f4c..8879df3 100644
--- a/gio/tests/gsubprocess-testprog.c
+++ b/gio/tests/gsubprocess-testprog.c
@@ -61,6 +61,16 @@ cat_mode (int argc,
 #endif
 }
 
+static gint
+sleep_forever_mode (int argc,
+		    char **argv)
+{
+  GMainLoop *loop;
+  
+  loop = g_main_loop_new (NULL, TRUE);
+  g_main_loop_run (loop);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -102,6 +112,8 @@ main (int argc, char **argv)
     return echo_stdout_and_stderr_mode (argc, argv);
   else if (strcmp (mode, "cat") == 0)
     return cat_mode (argc, argv);
+  else if (strcmp (mode, "sleep-forever") == 0)
+    return sleep_forever_mode (argc, argv);
   else
     {
       g_printerr ("Unknown MODE %s\n", argv[1]);
diff --git a/gio/tests/gsubprocess.c b/gio/tests/gsubprocess.c
index 0f8e8d7..63d6061 100644
--- a/gio/tests/gsubprocess.c
+++ b/gio/tests/gsubprocess.c
@@ -399,6 +399,57 @@ test_argv0 (void)
   */
 }
 
+static gboolean
+send_terminate (gpointer   user_data)
+{
+  GSubprocess *proc = user_data;
+
+  g_subprocess_terminate (proc);
+}
+
+static void
+on_request_quit_exited (GSubprocess   *self,
+			gpointer       user_data)
+{
+  g_main_loop_quit ((GMainLoop*)user_data);
+}
+
+static void
+test_terminate (void)
+{
+  GSubprocess *proc;
+  GError *local_error = NULL;
+  GError **error = &local_error;
+  GMainLoop *loop;
+  GSource *source;
+
+  proc = get_test_subprocess ("sleep-forever");
+
+  g_subprocess_start (proc, NULL, error);
+  g_assert_no_error (local_error);
+
+  loop = g_main_loop_new (NULL, TRUE);
+
+  source = g_subprocess_add_watch (proc, on_request_quit_exited, loop);
+
+  g_timeout_add_seconds (3, send_terminate, proc);
+
+  g_main_loop_run (loop);
+
+  (void)g_subprocess_query_success (proc, error);
+  g_assert_error (local_error, G_IO_ERROR, G_IO_ERROR_SUBPROCESS_EXIT_ABNORMAL);
+  g_clear_error (error);
+
+#ifdef G_OS_UNIX
+  {
+    int scode = g_subprocess_get_status_code (proc);
+    g_assert (WIFSIGNALED (scode) && WTERMSIG (scode) == 9);
+  }
+#endif
+
+  g_object_unref (proc);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -419,6 +470,7 @@ main (int argc, char **argv)
   g_test_add_func ("/gsubprocess/cat-utf8", test_cat_utf8);
   g_test_add_func ("/gsubprocess/cat-non-utf8", test_cat_non_utf8);
   g_test_add_func ("/gsubprocess/multi1", test_multi_1);
+  g_test_add_func ("/gsubprocess/terminate", test_terminate);
   /* g_test_add_func ("/gsubprocess/argv0", test_argv0); */
 
   return g_test_run ();