Process API improvements (again) (#1370)

* feat(process): add push_error
* refactor(process): use push_error for better errors
* style(process): consistent error messages
* refactor(process): reimplement process.strerror() with push_error
* refactor(process): implement close_fd only once
* refactor(process): rename process_handle to process_handle_t
* fix(process): prevent errors from a NULL error message
* refactor(process): refactor push_error into 2 functions
* fix(process): fix wrong error message
* fix(process): check if push_error_string actually pushed something
* refactor(process): make error messages descriptive
* fix(process): check for empty table instead of aborting
* refactor(process): make error messages descriptive on Windows
* refactor(process): rename process_stream_handle to process_stream_t
* refactor(process): fix wrong usage of process_handle_t
* fix(process): fix wrong type name
* refactor(process): incoporate kill_list_thread into process_kill_list_t
* refactor(process): make kill_list per-state data

src/api/process.c

@@ -24,13 +24,28 @@
 #define READ_BUF_SIZE 2048
 #define PROCESS_TERM_TRIES 3
 #define PROCESS_TERM_DELAY 50
+#define PROCESS_KILL_LIST_NAME "__process_kill_list__"
 
 #if _WIN32
-typedef HANDLE process_stream_handle;
+
-typedef HANDLE process_handle;
+typedef DWORD process_error_t;
+typedef HANDLE process_stream_t;
+typedef HANDLE process_handle_t;
+
+#define HANDLE_INVALID (INVALID_HANDLE_VALUE)
+#define PROCESS_GET_HANDLE(P) ((P)->process_information.hProcess)
+
+static volatile long PipeSerialNumber;
+
 #else
-typedef int process_stream_handle;
+
-typedef pid_t process_handle;
+typedef int process_error_t;
+typedef int process_stream_t;
+typedef pid_t process_handle_t;
+
+#define HANDLE_INVALID (0)
+#define PROCESS_GET_HANDLE(P) ((P)->pid)
+
 #endif
 
 typedef struct {
@@ -43,13 +58,13 @@ typedef struct {
     bool reading[2];
     char buffer[2][READ_BUF_SIZE];
   #endif
-  process_stream_handle child_pipes[3][2];
+  process_stream_t child_pipes[3][2];
 } process_t;
 
 typedef struct process_kill_s {
   int tries;
   uint32_t start_time;
-  process_handle handle;
+  process_handle_t handle;
   struct process_kill_s *next;
 } process_kill_t;
 
@@ -57,6 +72,7 @@ typedef struct {
   bool stop;
   SDL_mutex *mutex;
   SDL_cond *has_work, *work_done;
+  SDL_Thread *worker_thread;
   process_kill_t *head;
   process_kill_t *tail;
 } process_kill_list_t;
@@ -83,21 +99,24 @@ typedef enum {
   REDIRECT_PARENT = -3,
 } filed_e;
 
-static process_kill_list_t kill_list = { 0 };
+static void close_fd(process_stream_t *handle) {
-static SDL_Thread *kill_list_thread = NULL;
+  if (*handle) {
-
 #ifdef _WIN32
-  static volatile long PipeSerialNumber;
+    CloseHandle(*handle);
-  static void close_fd(HANDLE* handle) { if (*handle) CloseHandle(*handle); *handle = INVALID_HANDLE_VALUE; }
-  #define PROCESS_GET_HANDLE(P) ((P)->process_information.hProcess)
 #else
-  static void close_fd(int* fd) { if (*fd) close(*fd); *fd = 0; }
+    close(*handle);
-  #define PROCESS_GET_HANDLE(P) ((P)->pid)
 #endif
+    *handle = HANDLE_INVALID;
+  }
+}
+
+
+static int kill_list_worker(void *ud);
 
 
 static void kill_list_free(process_kill_list_t *list) {
   process_kill_t *node, *temp;
+  SDL_WaitThread(list->worker_thread, NULL);
   SDL_DestroyMutex(list->mutex);
   SDL_DestroyCond(list->has_work);
   SDL_DestroyCond(list->work_done);
@@ -107,10 +126,12 @@ static void kill_list_free(process_kill_list_t *list) {
     node = node->next;
     free(temp);
   }
+  memset(list, 0, sizeof(process_kill_list_t));
 }
 
 
 static bool kill_list_init(process_kill_list_t *list) {
+  memset(list, 0, sizeof(process_kill_list_t));
   list->mutex = SDL_CreateMutex();
   list->has_work = SDL_CreateCond();
   list->work_done = SDL_CreateCond();
@@ -120,6 +141,11 @@ static bool kill_list_init(process_kill_list_t *list) {
     kill_list_free(list);
     return false;
   }
+  list->worker_thread = SDL_CreateThread(kill_list_worker, "process_kill", list);
+  if (!list->worker_thread) {
+    kill_list_free(list);
+    return false;
+  }
   return true;
 }
 
@@ -157,7 +183,7 @@ static void kill_list_wait_all(process_kill_list_t *list) {
 }
 
 
-static void process_handle_close(process_handle *handle) {
+static void process_handle_close(process_handle_t *handle) {
 #ifdef _WIN32
   if (*handle) {
     CloseHandle(*handle);
@@ -168,7 +194,7 @@ static void process_handle_close(process_handle *handle) {
 }
 
 
-static bool process_handle_is_running(process_handle handle, int *status) {
+static bool process_handle_is_running(process_handle_t handle, int *status) {
 #ifdef _WIN32
   DWORD s;
   if (GetExitCodeProcess(handle, &s) && s != STILL_ACTIVE) {
@@ -188,7 +214,7 @@ static bool process_handle_is_running(process_handle handle, int *status) {
 }
 
 
-static bool process_handle_signal(process_handle handle, signal_e sig) {
+static bool process_handle_signal(process_handle_t handle, signal_e sig) {
 #if _WIN32
   switch(sig) {
     case SIGNAL_TERM: return GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, GetProcessId(handle));
@@ -252,6 +278,37 @@ static int kill_list_worker(void *ud) {
 }
 
 
+static int push_error_string(lua_State *L, process_error_t err) {
+#ifdef _WIN32
+  char *msg = NULL;
+  FormatMessageA(FORMAT_MESSAGE_FROM_SYSTEM
+                | FORMAT_MESSAGE_ALLOCATE_BUFFER
+                | FORMAT_MESSAGE_IGNORE_INSERTS,
+                NULL,
+                err,
+                MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                (char *) &msg,
+                0,
+                NULL);
+  if (!msg)
+    return 0;
+
+  lua_pushstring(L, msg);
+  LocalFree(msg);
+#else
+  lua_pushstring(L, strerror(err));
+#endif
+  return 1;
+}
+
+static void push_error(lua_State *L, const char *extra, process_error_t err) {
+  const char *msg = "unknown error";
+  extra = extra != NULL ? extra : "error";
+  if (push_error_string(L, err))
+    msg = lua_tostring(L, -1);
+  lua_pushfstring(L, "%s: %s (%d)", extra, msg, err);
+}
+
 static bool poll_process(process_t* proc, int timeout) {
   uint32_t ticks;
 
@@ -296,6 +353,10 @@ static int process_start(lua_State* L) {
       lua_pushinteger(L, (int)lua_objlen(L, 1));
     #endif
     cmd_len = luaL_checknumber(L, -1); lua_pop(L, 1);
+    if (!cmd_len)
+      // we have not allocated anything here yet, so we can skip cleanup code
+      // don't do this anywhere else!
+      return luaL_argerror(L, 1,"table cannot be empty");
     for (size_t i = 1; i <= cmd_len; ++i) {
       lua_pushinteger(L, i);
       lua_rawget(L, 1);
@@ -306,9 +367,6 @@ static int process_start(lua_State* L) {
     cmd[0] = luaL_checkstring(L, 1);
     cmd_len = 1;
   }
-  // this should never trip
-  // but if it does we are in deep trouble
-  assert(cmd[0]);
 
   if (arg_len > 1) {
     lua_getfield(L, 2, "env");
@@ -334,7 +392,7 @@ static int process_start(lua_State* L) {
     lua_getfield(L, 2, "stderr");  new_fds[STDERR_FD] = luaL_optnumber(L, -1, STDERR_FD);
     for (int stream = STDIN_FD; stream <= STDERR_FD; ++stream) {
       if (new_fds[stream] > STDERR_FD || new_fds[stream] < REDIRECT_PARENT) {
-        lua_pushfstring(L, "redirect to handles, FILE* and paths are not supported");
+        lua_pushfstring(L, "error: redirect to handles, FILE* and paths are not supported");
         retval = -1;
         goto cleanup;
       }
@@ -368,20 +426,22 @@ static int process_start(lua_State* L) {
             self->child_pipes[i][0] = CreateNamedPipeA(pipeNameBuffer, PIPE_ACCESS_INBOUND | FILE_FLAG_OVERLAPPED,
               PIPE_TYPE_BYTE | PIPE_WAIT, 1, READ_BUF_SIZE, READ_BUF_SIZE, 0, NULL);
             if (self->child_pipes[i][0] == INVALID_HANDLE_VALUE) {
-              lua_pushfstring(L, "Error creating read pipe: %d.", GetLastError());
+              push_error(L, "cannot create pipe", GetLastError());
               retval = -1;
               goto cleanup;
             }
             self->child_pipes[i][1] = CreateFileA(pipeNameBuffer, GENERIC_WRITE, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
             if (self->child_pipes[i][1] == INVALID_HANDLE_VALUE) {
+              // prevent CloseHandle from messing up error codes
+              DWORD err = GetLastError();
               CloseHandle(self->child_pipes[i][0]);
-              lua_pushfstring(L, "Error creating write pipe: %d.", GetLastError());
+              push_error(L, "cannot open pipe", err);
               retval = -1;
               goto cleanup;
             }
             if (!SetHandleInformation(self->child_pipes[i][i == STDIN_FD ? 1 : 0], HANDLE_FLAG_INHERIT, 0) ||
                 !SetHandleInformation(self->child_pipes[i][i == STDIN_FD ? 0 : 1], HANDLE_FLAG_INHERIT, 1)) {
-                  lua_pushfstring(L, "Error inheriting pipes: %d.", GetLastError());
+                  push_error(L, "cannot set pipe permission", GetLastError());
                   retval = -1;
                   goto cleanup;
             }
@@ -445,7 +505,7 @@ static int process_start(lua_State* L) {
     if (env_len > 0)
       MultiByteToWideChar(CP_UTF8, MB_PRECOMPOSED, environmentBlock, offset, (LPWSTR)wideEnvironmentBlock, sizeof(wideEnvironmentBlock));
     if (!CreateProcess(NULL, commandLine, NULL, NULL, true, (detach ? DETACHED_PROCESS : CREATE_NO_WINDOW) | CREATE_UNICODE_ENVIRONMENT, env_len > 0 ? wideEnvironmentBlock : NULL, cwd, &siStartInfo, &self->process_information)) {
-      lua_pushfstring(L, "Error creating a process: %d.", GetLastError());
+      push_error(L, NULL, GetLastError());
       retval = -1;
       goto cleanup;
     }
@@ -457,7 +517,7 @@ static int process_start(lua_State* L) {
     int control_pipe[2] = { 0 };
     for (int i = 0; i < 3; ++i) { // Make only the parents fd's non-blocking. Children should block.
       if (pipe(self->child_pipes[i]) || fcntl(self->child_pipes[i][i == STDIN_FD ? 1 : 0], F_SETFL, O_NONBLOCK) == -1) {
-        lua_pushfstring(L, "Error creating pipes: %s", strerror(errno));
+        push_error(L, "cannot create pipe", errno);
         retval = -1;
         goto cleanup;
       }
@@ -476,7 +536,7 @@ static int process_start(lua_State* L) {
 
     self->pid = (long)fork();
     if (self->pid < 0) {
-      lua_pushfstring(L, "Error running fork: %s.", strerror(errno));
+      push_error(L, "cannot create child process", errno);
       retval = -1;
       goto cleanup;
     } else if (!self->pid) {
@@ -533,7 +593,7 @@ static int process_start(lua_State* L) {
     free((char*)env_values[i]);
   }
   for (int stream = 0; stream < 3; ++stream) {
-    process_stream_handle* pipe = &self->child_pipes[stream][stream == STDIN_FD ? 0 : 1];
+    process_stream_t* pipe = &self->child_pipes[stream][stream == STDIN_FD ? 0 : 1];
     if (*pipe) {
       close_fd(pipe);
     }
@@ -549,7 +609,7 @@ static int g_read(lua_State* L, int stream, unsigned long read_size) {
   process_t* self = (process_t*) luaL_checkudata(L, 1, API_TYPE_PROCESS);
   long length = 0;
   if (stream != STDOUT_FD && stream != STDERR_FD)
-    return luaL_error(L, "redirect to handles, FILE* and paths are not supported");
+    return luaL_error(L, "error: redirect to handles, FILE* and paths are not supported");
   #if _WIN32
     int writable_stream_idx = stream - 1;
     if (self->reading[writable_stream_idx] || !ReadFile(self->child_pipes[stream][0], self->buffer[writable_stream_idx], READ_BUF_SIZE, NULL, &self->overlapped[writable_stream_idx])) {
@@ -597,9 +657,9 @@ static int f_write(lua_State* L) {
   #if _WIN32
     DWORD dwWritten;
     if (!WriteFile(self->child_pipes[STDIN_FD][1], data, data_size, &dwWritten, NULL)) {
-      int lastError = GetLastError();
+      push_error(L, NULL, GetLastError());
       signal_process(self, SIGNAL_TERM);
-      return luaL_error(L, "error writing to process: %d", lastError);
+      return lua_error(L);
     }
     length = dwWritten;
   #else
@@ -607,9 +667,9 @@ static int f_write(lua_State* L) {
     if (length < 0 && (errno == EAGAIN || errno == EWOULDBLOCK))
       length = 0;
     else if (length < 0) {
-      const char* lastError = strerror(errno);
+      push_error(L, "cannot write to child process", errno);
       signal_process(self, SIGNAL_TERM);
-      return luaL_error(L, "error writing to process: %s", lastError);
+      return lua_error(L);
     }
   #endif
   lua_pushinteger(L, length);
@@ -626,15 +686,7 @@ static int f_close_stream(lua_State* L) {
 
 // Generic stuff below here.
 static int process_strerror(lua_State* L) {
-  #if _WIN32
+  return push_error_string(L, luaL_checknumber(L, 1));
-    return 1;
-  #endif
-  int error_code = luaL_checknumber(L, 1);
-  if (error_code < 0)
-    lua_pushstring(L, strerror(error_code));
-  else
-    lua_pushnil(L);
-  return 1;
 }
 
 static int f_tostring(lua_State* L) {
@@ -688,30 +740,32 @@ static int f_kill(lua_State* L) { return self_signal(L, SIGNAL_KILL); }
 static int f_interrupt(lua_State* L) { return self_signal(L, SIGNAL_INTERRUPT); }
 
 static int f_gc(lua_State* L) {
+  process_kill_list_t *list = NULL;
+  process_kill_t *p = NULL;
   process_t* self = (process_t*) luaL_checkudata(L, 1, API_TYPE_PROCESS);
 
-  if (poll_process(self, 0) && !self->detached) {
+  // get the kill_list for the lua_State
-    // attempt to kill the process if not detached
+  if (lua_getfield(L, LUA_REGISTRYINDEX, PROCESS_KILL_LIST_NAME) == LUA_TUSERDATA)
-    process_kill_t *p;
+    list = (process_kill_list_t *) lua_touserdata(L, -1);
 
+  if (poll_process(self, 0) && !self->detached) {
+    // attempt to kill the process if still running and not detached
     signal_process(self, SIGNAL_TERM);
-    p = malloc(sizeof(process_kill_t));
+    if (!list || !list->worker_thread || !(p = malloc(sizeof(process_kill_t)))) {
-    if (!p || !kill_list_thread) {
+      // use synchronous waiting
-      // if we can't allocate, we'll use the old method
+      if (poll_process(self, PROCESS_TERM_DELAY)) {
-      poll_process(self, PROCESS_TERM_DELAY);
-      if (self->running) {
         signal_process(self, SIGNAL_KILL);
         poll_process(self, PROCESS_TERM_DELAY);
       }
     } else {
-      // send the handle to a queue for asynchronous waiting
+      // put the handle into a queue for asynchronous waiting
       p->handle = PROCESS_GET_HANDLE(self);
       p->start_time = SDL_GetTicks();
       p->tries = 1;
-      SDL_LockMutex(kill_list.mutex);
+      SDL_LockMutex(list->mutex);
-      kill_list_push(&kill_list, p);
+      kill_list_push(list, p);
-      SDL_CondSignal(kill_list.has_work);
+      SDL_CondSignal(list->has_work);
-      SDL_UnlockMutex(kill_list.mutex);
+      SDL_UnlockMutex(list->mutex);
     }
   }
   close_fd(&self->child_pipes[STDIN_FD ][1]);
@@ -727,9 +781,13 @@ static int f_running(lua_State* L) {
 }
 
 static int process_gc(lua_State *L) {
-  kill_list_wait_all(&kill_list);
+  process_kill_list_t *list = NULL;
-  SDL_WaitThread(kill_list_thread, NULL);
+  // get the kill_list for the lua_State
-  kill_list_free(&kill_list);
+  if (lua_getfield(L, LUA_REGISTRYINDEX, PROCESS_KILL_LIST_NAME) == LUA_TUSERDATA) {
+    list = (process_kill_list_t *) lua_touserdata(L, -1);
+    kill_list_wait_all(list);
+    kill_list_free(list);
+  }
   return 0;
 }
 
@@ -758,8 +816,11 @@ static const struct luaL_Reg lib[] = {
 };
 
 int luaopen_process(lua_State *L) {
-  if (kill_list_init(&kill_list))
+  process_kill_list_t *list = lua_newuserdata(L, sizeof(process_kill_list_t));
-    kill_list_thread = SDL_CreateThread(kill_list_worker, "process_kill", &kill_list);
+  if (kill_list_init(list))
+    lua_setfield(L, LUA_REGISTRYINDEX, PROCESS_KILL_LIST_NAME);
+  else
+    lua_pop(L, 1); // discard the list
 
   // create the process metatable
   luaL_newmetatable(L, API_TYPE_PROCESS);