[grilo-plugins] lua-factory: watchdog for lua-sources

[Bastien Nocera <hadess src gnome org>]

commit f2860cd6db83158c47a07d46d7b24f2218cd9d13
Author: Victor Toso <me victortoso com>
Date:   Sun Mar 13 17:09:47 2016 +0100

    lua-factory: watchdog for lua-sources
    
    This commit allow us to track broken sources such as:
    * sources that never call for grl.callback()
    * sources that might call grl.callback() after the operation is
      already finished
    
    This is done by using a userdata in the top of the stack as a watchdog
    for the current operation. We always push this userdata in
    grl_lua_library_pcall function and it will be always freed by Lua's
    garbage collection.
    
    By attaching a finalize function into this userdata, we can check at
    the time that lua_gc is called if the state of Operation is correct.
    
    This idea was introduced by Morse in d809be39ed2bb so I modify those
    functions to work as the watchdog we need instead of grl_l_callback
    closure.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763046

 src/lua-factory/grl-lua-library-operations.c |  143 +++++++++++++++++++++++++-
 src/lua-factory/grl-lua-library.c            |   82 ---------------
 2 files changed, 142 insertions(+), 83 deletions(-)
---
diff --git a/src/lua-factory/grl-lua-library-operations.c b/src/lua-factory/grl-lua-library-operations.c
index 961417d..47e256b 100644
--- a/src/lua-factory/grl-lua-library-operations.c
+++ b/src/lua-factory/grl-lua-library-operations.c
@@ -435,6 +435,139 @@ priv_state_operations_update (lua_State *L,
   GRL_ERROR ("Ongoig operation not found (op-id: %d)", os->operation_id);
 }
 
+/* ============== Watchdog related ========================================= */
+
+/**
+ * grl_util_operation_spec_gc
+ *
+ * This function is called when Lua GC is about to collect the userdata
+ * representing OperationSpec. Here we check that the finishing callback
+ * was done and free the memory.
+ *
+ * @L: LuaState where the data is stored.
+ * @return: 0, as the number of objects left on stack.
+ *          It is important, for Lua stack to not be corrupted.
+ **/
+static int
+watchdog_operation_gc (lua_State *L)
+{
+  guint *pid = lua_touserdata (L, 1);
+  LuaSourceState state = priv_state_operations_source_get_state (L, *pid);
+  OperationSpec *os = priv_state_operations_source_get_op_data (L, *pid);
+  OperationSpec *current_os = priv_state_current_op_get_op_data (L);
+  const char *type;
+
+  GRL_DEBUG ("%s | %s (op-id: %u) current state is: %s (num-async-op: %u)", __func__,
+             grl_source_get_id (os->source),
+             os->operation_id,
+             source_op_state_str[state],
+             os->lua_source_waiting_ops);
+
+  switch (state) {
+  case LUA_SOURCE_RUNNING:
+    /* Check if waiting for async op, otherwise it is an error */
+    if (os->lua_source_waiting_ops > 0) {
+      GRL_DEBUG ("%s | %s (op-id: %u) awaiting for %u async operations", __func__,
+                 grl_source_get_id (os->source),
+                 os->operation_id,
+                 os->lua_source_waiting_ops);
+      pid = NULL;
+      return 0;
+    }
+    break;
+
+  case LUA_SOURCE_WAITING:
+    /* Waiting for async op to finish, that's fine! */
+    pid = NULL;
+    return 0;
+    break;
+
+  case LUA_SOURCE_FINALIZED:
+    if (os->lua_source_waiting_ops > 0) {
+      GRL_WARNING ("Source '%s' is broken, as the finishing callback was called "
+                   "while %u operations are still ongoing",
+                   grl_source_get_id (os->source),
+                   os->lua_source_waiting_ops);
+      pid = NULL;
+      return 0;
+    }
+
+    priv_state_operations_remove_source_state (L, os->operation_id);
+    if (current_os->operation_id == os->operation_id)
+      priv_state_current_op_remove (L);
+    free_operation_spec (os);
+    pid = NULL;
+    return 0;
+    break;
+
+  default:
+    g_assert_not_reached ();
+  }
+
+  switch (os->op_type) {
+  case LUA_SEARCH:
+    type = "search";
+    break;
+  case LUA_BROWSE:
+    type = "browse";
+    break;
+  case LUA_QUERY:
+    type = "query";
+    break;
+  case LUA_RESOLVE:
+    type = "resolve";
+    break;
+  default:
+    g_assert_not_reached ();
+  }
+
+  GRL_WARNING ("Source '%s' is broken, as the finishing "
+               "callback was not called for %s operation",
+               grl_source_get_id (os->source),
+               type);
+  switch (os->op_type) {
+  case LUA_RESOLVE:
+    os->cb.resolve (os->source, os->operation_id, os->media, os->user_data, NULL);
+    break;
+
+  default:
+    os->cb.result (os->source, os->operation_id, NULL,
+                   0, os->user_data, NULL);
+  }
+
+  free_operation_spec (os);
+  pid = NULL;
+  return 0;
+}
+
+/**
+ * push_operation_spec_userdata
+ *
+ * Creates a userdata on top of the Lua stack, with the GC function
+ * assigned to it.
+ *
+ * @L: LuaState where the data is stored.
+ * @os: OperationSpec from which to create userdata.
+ * @return: Nothing.
+ **/
+static void
+watchdog_operation_push (lua_State *L, guint operation_id)
+{
+  gint *pid = lua_newuserdata (L, sizeof (guint));
+  *pid = operation_id;
+
+  /* create the metatable */
+  lua_createtable (L, 0, 1);
+  /* push the __gc key string */
+  lua_pushstring (L, "__gc");
+  /* push the __gc metamethod */
+  lua_pushcfunction (L, watchdog_operation_gc);
+  /* set the __gc field in the metatable */
+  lua_settable (L, -3);
+  /* set table as the metatable of the userdata */
+  lua_setmetatable (L, -2);
+}
+
 /* =========================================================================
  * Exported functions ======================================================
  * ========================================================================= */
@@ -548,7 +681,14 @@ grl_lua_operations_pcall (lua_State *L,
              grl_source_get_id (os->source),
              os->operation_id);
 
-  if (lua_pcall (L, nargs, 0, 0)) {
+  /* We control the GC during our function calls due the fact that we rely that
+   * watchdog's finalizer function is called after the lua_pcall, otherwise the
+   * watchdog fails to check for errors in the source and could lead to bugs */
+  lua_gc (L, LUA_GCSTOP, 0);
+
+  watchdog_operation_push (L, os->operation_id);
+  grl_lua_operations_set_source_state (L, LUA_SOURCE_RUNNING, os);
+  if (lua_pcall (L, nargs + 1, 0, 0)) {
     *err = g_error_new_literal (GRL_CORE_ERROR,
                                 os->error_code,
                                 lua_tolstring (L, -1, NULL));
@@ -556,6 +696,7 @@ grl_lua_operations_pcall (lua_State *L,
   }
 
   lua_gc (L, LUA_GCCOLLECT, 0);
+  lua_gc (L, LUA_GCRESTART, 0);
   return (*err == NULL);
 }
 
diff --git a/src/lua-factory/grl-lua-library.c b/src/lua-factory/grl-lua-library.c
index 62401fc..b5345af 100644
--- a/src/lua-factory/grl-lua-library.c
+++ b/src/lua-factory/grl-lua-library.c
@@ -1668,88 +1668,6 @@ luaopen_grilo (lua_State *L)
 /* ======= Lua-Library and Lua-Factory utilities ============= */
 
 /**
- * grl_util_operation_spec_gc
- *
- * This function is called when Lua GC is about to collect the userdata
- * representing OperationSpec. Here we check that the finishing callback
- * was done and free the memory.
- *
- * @L: LuaState where the data is stored.
- * @return: 0, as the number of objects left on stack.
- *          It is important, for Lua stack to not be corrupted.
- **/
-static int
-grl_util_operation_spec_gc (lua_State *L)
-{
-  OperationSpec **userdata = lua_touserdata (L, 1);
-  OperationSpec *os = *userdata;
-
-  if (grl_lua_operations_get_current_op (L)) {
-
-    const char *type;
-    switch (os->op_type) {
-    case LUA_SEARCH:
-      type = "search";
-      break;
-    case LUA_BROWSE:
-      type = "browse";
-      break;
-    case LUA_QUERY:
-      type = "query";
-      break;
-    case LUA_RESOLVE:
-      type = "resolve";
-      break;
-    default:
-      g_assert_not_reached ();
-    }
-
-    GRL_WARNING ("Source '%s' is broken, as the finishing "
-                 "callback was not called for %s operation", grl_source_get_id (os->source), type);
-    switch (os->op_type) {
-    case LUA_RESOLVE:
-      os->cb.resolve (os->source, os->operation_id, NULL, os->user_data, NULL);
-      break;
-
-    default:
-      os->cb.result (os->source, os->operation_id, NULL,
-                     0, os->user_data, NULL);
-    }
-  }
-
-  grl_lua_operations_set_source_state (L, LUA_SOURCE_FINALIZED, os);
-  *userdata = NULL;
-  return 0;
-}
-/**
- * push_operation_spec_userdata
- *
- * Creates a userdata on top of the Lua stack, with the GC function
- * assigned to it.
- *
- * @L: LuaState where the data is stored.
- * @os: OperationSpec from which to create userdata.
- * @return: Nothing.
- **/
-static void
-push_operation_spec_userdata (lua_State *L, OperationSpec *os)
-{
-  OperationSpec **userdata = lua_newuserdata (L, sizeof (OperationSpec *));
-  *userdata = os;
-
-  /* create the metatable */
-  lua_createtable (L, 0, 1);
-  /* push the __gc key string */
-  lua_pushstring (L, "__gc");
-  /* push the __gc metamethod */
-  lua_pushcclosure (L, grl_util_operation_spec_gc, 0);
-  /* set the __gc field in the metatable */
-  lua_settable (L, -3);
-  /* set table as the metatable of the userdata */
-  lua_setmetatable (L, -2);
-}
-
-/**
  * grl_lua_library_save_goa_data
  *
  * @L: LuaState where the data will be stored.