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
This commit is contained in:
Takase 2023-04-18 02:56:04 +08:00 committed by George Sokianos
parent 3c60c1c7f1
commit e2a582d5fd
1 changed files with 121 additions and 60 deletions

View File

@ -24,13 +24,28 @@
#define READ_BUF_SIZE 2048 #define READ_BUF_SIZE 2048
#define PROCESS_TERM_TRIES 3 #define PROCESS_TERM_TRIES 3
#define PROCESS_TERM_DELAY 50 #define PROCESS_TERM_DELAY 50
#define PROCESS_KILL_LIST_NAME "__process_kill_list__"
#if _WIN32 #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 #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 #endif
typedef struct { typedef struct {
@ -43,13 +58,13 @@ typedef struct {
bool reading[2]; bool reading[2];
char buffer[2][READ_BUF_SIZE]; char buffer[2][READ_BUF_SIZE];
#endif #endif
process_stream_handle child_pipes[3][2]; process_stream_t child_pipes[3][2];
} process_t; } process_t;
typedef struct process_kill_s { typedef struct process_kill_s {
int tries; int tries;
uint32_t start_time; uint32_t start_time;
process_handle handle; process_handle_t handle;
struct process_kill_s *next; struct process_kill_s *next;
} process_kill_t; } process_kill_t;
@ -57,6 +72,7 @@ typedef struct {
bool stop; bool stop;
SDL_mutex *mutex; SDL_mutex *mutex;
SDL_cond *has_work, *work_done; SDL_cond *has_work, *work_done;
SDL_Thread *worker_thread;
process_kill_t *head; process_kill_t *head;
process_kill_t *tail; process_kill_t *tail;
} process_kill_list_t; } process_kill_list_t;
@ -83,21 +99,24 @@ typedef enum {
REDIRECT_PARENT = -3, REDIRECT_PARENT = -3,
} filed_e; } 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 #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 #else
static void close_fd(int* fd) { if (*fd) close(*fd); *fd = 0; } close(*handle);
#define PROCESS_GET_HANDLE(P) ((P)->pid)
#endif #endif
*handle = HANDLE_INVALID;
}
}
static int kill_list_worker(void *ud);
static void kill_list_free(process_kill_list_t *list) { static void kill_list_free(process_kill_list_t *list) {
process_kill_t *node, *temp; process_kill_t *node, *temp;
SDL_WaitThread(list->worker_thread, NULL);
SDL_DestroyMutex(list->mutex); SDL_DestroyMutex(list->mutex);
SDL_DestroyCond(list->has_work); SDL_DestroyCond(list->has_work);
SDL_DestroyCond(list->work_done); SDL_DestroyCond(list->work_done);
@ -107,10 +126,12 @@ static void kill_list_free(process_kill_list_t *list) {
node = node->next; node = node->next;
free(temp); free(temp);
} }
memset(list, 0, sizeof(process_kill_list_t));
} }
static bool kill_list_init(process_kill_list_t *list) { static bool kill_list_init(process_kill_list_t *list) {
memset(list, 0, sizeof(process_kill_list_t));
list->mutex = SDL_CreateMutex(); list->mutex = SDL_CreateMutex();
list->has_work = SDL_CreateCond(); list->has_work = SDL_CreateCond();
list->work_done = 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); kill_list_free(list);
return false; 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; 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 #ifdef _WIN32
if (*handle) { if (*handle) {
CloseHandle(*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 #ifdef _WIN32
DWORD s; DWORD s;
if (GetExitCodeProcess(handle, &s) && s != STILL_ACTIVE) { 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 #if _WIN32
switch(sig) { switch(sig) {
case SIGNAL_TERM: return GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT, GetProcessId(handle)); 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) { static bool poll_process(process_t* proc, int timeout) {
uint32_t ticks; uint32_t ticks;
@ -296,6 +353,10 @@ static int process_start(lua_State* L) {
lua_pushinteger(L, (int)lua_objlen(L, 1)); lua_pushinteger(L, (int)lua_objlen(L, 1));
#endif #endif
cmd_len = luaL_checknumber(L, -1); lua_pop(L, 1); 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) { for (size_t i = 1; i <= cmd_len; ++i) {
lua_pushinteger(L, i); lua_pushinteger(L, i);
lua_rawget(L, 1); lua_rawget(L, 1);
@ -306,9 +367,6 @@ static int process_start(lua_State* L) {
cmd[0] = luaL_checkstring(L, 1); cmd[0] = luaL_checkstring(L, 1);
cmd_len = 1; cmd_len = 1;
} }
// this should never trip
// but if it does we are in deep trouble
assert(cmd[0]);
if (arg_len > 1) { if (arg_len > 1) {
lua_getfield(L, 2, "env"); 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); lua_getfield(L, 2, "stderr"); new_fds[STDERR_FD] = luaL_optnumber(L, -1, STDERR_FD);
for (int stream = STDIN_FD; stream <= STDERR_FD; ++stream) { for (int stream = STDIN_FD; stream <= STDERR_FD; ++stream) {
if (new_fds[stream] > STDERR_FD || new_fds[stream] < REDIRECT_PARENT) { 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; retval = -1;
goto cleanup; 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, 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); PIPE_TYPE_BYTE | PIPE_WAIT, 1, READ_BUF_SIZE, READ_BUF_SIZE, 0, NULL);
if (self->child_pipes[i][0] == INVALID_HANDLE_VALUE) { 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; retval = -1;
goto cleanup; goto cleanup;
} }
self->child_pipes[i][1] = CreateFileA(pipeNameBuffer, GENERIC_WRITE, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); 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) { 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]); CloseHandle(self->child_pipes[i][0]);
lua_pushfstring(L, "Error creating write pipe: %d.", GetLastError()); push_error(L, "cannot open pipe", err);
retval = -1; retval = -1;
goto cleanup; goto cleanup;
} }
if (!SetHandleInformation(self->child_pipes[i][i == STDIN_FD ? 1 : 0], HANDLE_FLAG_INHERIT, 0) || 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)) { !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; retval = -1;
goto cleanup; goto cleanup;
} }
@ -445,7 +505,7 @@ static int process_start(lua_State* L) {
if (env_len > 0) if (env_len > 0)
MultiByteToWideChar(CP_UTF8, MB_PRECOMPOSED, environmentBlock, offset, (LPWSTR)wideEnvironmentBlock, sizeof(wideEnvironmentBlock)); 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)) { 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; retval = -1;
goto cleanup; goto cleanup;
} }
@ -457,7 +517,7 @@ static int process_start(lua_State* L) {
int control_pipe[2] = { 0 }; int control_pipe[2] = { 0 };
for (int i = 0; i < 3; ++i) { // Make only the parents fd's non-blocking. Children should block. 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) { 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; retval = -1;
goto cleanup; goto cleanup;
} }
@ -476,7 +536,7 @@ static int process_start(lua_State* L) {
self->pid = (long)fork(); self->pid = (long)fork();
if (self->pid < 0) { if (self->pid < 0) {
lua_pushfstring(L, "Error running fork: %s.", strerror(errno)); push_error(L, "cannot create child process", errno);
retval = -1; retval = -1;
goto cleanup; goto cleanup;
} else if (!self->pid) { } else if (!self->pid) {
@ -533,7 +593,7 @@ static int process_start(lua_State* L) {
free((char*)env_values[i]); free((char*)env_values[i]);
} }
for (int stream = 0; stream < 3; ++stream) { 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) { if (*pipe) {
close_fd(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); process_t* self = (process_t*) luaL_checkudata(L, 1, API_TYPE_PROCESS);
long length = 0; long length = 0;
if (stream != STDOUT_FD && stream != STDERR_FD) 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 #if _WIN32
int writable_stream_idx = stream - 1; 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])) { 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 #if _WIN32
DWORD dwWritten; DWORD dwWritten;
if (!WriteFile(self->child_pipes[STDIN_FD][1], data, data_size, &dwWritten, NULL)) { 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); signal_process(self, SIGNAL_TERM);
return luaL_error(L, "error writing to process: %d", lastError); return lua_error(L);
} }
length = dwWritten; length = dwWritten;
#else #else
@ -607,9 +667,9 @@ static int f_write(lua_State* L) {
if (length < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) if (length < 0 && (errno == EAGAIN || errno == EWOULDBLOCK))
length = 0; length = 0;
else if (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); signal_process(self, SIGNAL_TERM);
return luaL_error(L, "error writing to process: %s", lastError); return lua_error(L);
} }
#endif #endif
lua_pushinteger(L, length); lua_pushinteger(L, length);
@ -626,15 +686,7 @@ static int f_close_stream(lua_State* L) {
// Generic stuff below here. // Generic stuff below here.
static int process_strerror(lua_State* L) { 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) { 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_interrupt(lua_State* L) { return self_signal(L, SIGNAL_INTERRUPT); }
static int f_gc(lua_State* L) { 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); 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); 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); signal_process(self, SIGNAL_KILL);
poll_process(self, PROCESS_TERM_DELAY); poll_process(self, PROCESS_TERM_DELAY);
} }
} else { } 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->handle = PROCESS_GET_HANDLE(self);
p->start_time = SDL_GetTicks(); p->start_time = SDL_GetTicks();
p->tries = 1; 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]); 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) { 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; return 0;
} }
@ -758,8 +816,11 @@ static const struct luaL_Reg lib[] = {
}; };
int luaopen_process(lua_State *L) { 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 // create the process metatable
luaL_newmetatable(L, API_TYPE_PROCESS); luaL_newmetatable(L, API_TYPE_PROCESS);