Nguyễn Gia Phong: 5 Rework run_process to base on posix_spawn Use the proper signal SIGTERM to kill shellpipe Use more explicit types for PID in the API Match sentinality with API Remove no-op free by arena allocator 8 files changed, 69 insertions(+), 41 deletions(-)
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~mil/mepo-devel/patches/54281/mbox | git am -3Learn more about email & git
Until the next Zig release, std.process.Child lacks the support for process group ID for the sync shellpipe process family to be properly killed. This rewrite also reset the spawned process's signal mask so it no longer inherit from mepo. --- Base commit: 23c8efa8f0dbfe8adfe2a035380ee72727fede99 on top of branch cnx-shellpipe-plus-rebase. src/api/shellpipe_sync.zig | 4 +- src/util/utilprocess.zig | 95 +++++++++++++++++++++++++------------- 2 files changed, 64 insertions(+), 35 deletions(-)
Looks good to me and all seems functional in my testing. Thanks for the followup work on the libc implementation - have applied to cnx-shellpipe-plus-rebase branch & merged the complete work on that branch to master. This is a vast improvement in UX over the previous sync forking model. Issue with sigmask inheritance makes sense as well and glad to see we're able to use SIGTERM now properly.
diff --git a/src/api/shellpipe_sync.zig b/src/api/shellpipe_sync.zig index 750c2885dd77..9d7517553787 100644 --- a/src/api/shellpipe_sync.zig +++ b/src/api/shellpipe_sync.zig @@ -41,7 +41,6 @@ fn run_sync_shellpipe(userdata: ?*anyopaque) callconv(.C) c_int { defer request.mepo.allocator.destroy(request); run_shellpipe(request.mepo, null, request.cmd) catch |err| utildbg.log("Error running sync shellpipe: {}\n", .{err}); - request.mepo.shellpipe_sync_pid = null; return 0; } @@ -53,7 +52,8 @@ fn kill_sync_shellpipe(mepo: *Mepo) types.Pending { utildbg.log("Killing sync shellpipe process {}\n", .{pid}) else |err| utildbg.log("Error killing sync shellpipe process {}: {}\n", .{ pid, err }); - // No need to wait for termination as run_shellpipe already handles HUP events. + // Process termination is waited by utilprocess.run_process + // in the other thread. return .Redraw; } else return .None; } diff --git a/src/util/utilprocess.zig b/src/util/utilprocess.zig index 94e4609f7282..fd05ec37fdf5 100644 --- a/src/util/utilprocess.zig +++ b/src/util/utilprocess.zig @@ -1,45 +1,74 @@ +const c = @cImport({ + @cInclude("spawn.h"); +}); const std = @import("std"); const utildbg = @import("../util/utildbg.zig"); -// In the case that []const u8 is returned, it's the callers responsibility -// to free the return value +// Executes the command cmd with environment variables in env_map. +// When spawned without any error, returns null if the process terminates early, +// otherwise stdout captured in a slice owned by the caller. pub fn run_subprocess( allocator: std.mem.Allocator, cmd: []const u8, - env_vars: *const std.process.EnvMap, + env_map: *const std.process.EnvMap, set_process_id_opt_ptr_opt: ?*?std.process.Child.Id, ) !?[]const u8 { + var pid: std.c.pid_t = undefined; + defer { + if (set_process_id_opt_ptr_opt) |pid_ptr| + pid_ptr.* = null; + } // Stop blocking the UI + + var actions: c.posix_spawn_file_actions_t = undefined; + if (c.posix_spawn_file_actions_init(&actions) != 0) + return error.PosixSpawnFileActionsInitFailed;
Forgot to mention, this kind of makeshift error handling is for brevity's sake. On the other hand I'm not sure make the log useful without a call stack dump. On 2024-08-05 at 16:18+09:00, Nguyễn Gia Phong wrote:
+ defer if (c.posix_spawn_file_actions_destroy(&actions) != 0) unreachable; + const pipe = try std.posix.pipe(); + errdefer std.posix.close(pipe[0]); + errdefer std.posix.close(pipe[1]); + // The read end of the pipe is unused by the spawned process. + if (c.posix_spawn_file_actions_addclose(&actions, pipe[0]) != 0) + return error.PosixSpawnFileActionsAddCloseFailed; + // Redirect stdout to the write end of the pipe. + if (c.posix_spawn_file_actions_adddup2(&actions, pipe[1], 1) != 0) + return error.PosixSpawnFileActionsAddDup2Failed; + + var attr: c.posix_spawnattr_t = undefined; + if (c.posix_spawnattr_init(&attr) != 0) + return error.PosixSpawnAttrInitFailed; + defer if (c.posix_spawnattr_destroy(&attr) != 0) unreachable; + const flags = c.POSIX_SPAWN_SETSIGMASK | + c.POSIX_SPAWN_SETSIGDEF | + c.POSIX_SPAWN_SETPGROUP; + if (c.posix_spawnattr_setflags(&attr, flags) != 0) + return error.PosixSpawnAttrSetFlagsFailed; + if (c.posix_spawnattr_setpgroup(&attr, 0) != 0) + return error.PosixSpawnAttrSetPgroupFailed; + var arena = std.heap.ArenaAllocator.init(allocator); defer arena.deinit(); - const args = [_][]const u8{ "sh", "-c", cmd }; - var child = std.process.Child.init(&args, arena.allocator()); - child.pgid = 0; - child.allocator = arena.allocator(); - child.argv = args[0..]; - child.stdout_behavior = .Pipe; - child.stderr_behavior = .Pipe; - child.env_map = env_vars; - var stdout = std.ArrayList(u8).init(arena.allocator()); - var stderr = std.ArrayList(u8).init(arena.allocator()); - errdefer { - stdout.deinit(); - stderr.deinit(); - } - try child.spawn(); - if (set_process_id_opt_ptr_opt != null) { - set_process_id_opt_ptr_opt.?.* = child.id; - } - try child.collectOutput(&stdout, &stderr, 10000 * 1024); - const term = try child.wait(); - switch (term) { - .Exited => |exitcode| { - if (exitcode == 0) { - return try stdout.toOwnedSlice(); - } - }, - else => { - utildbg.log("Early termination of async_shellpipe (via signal, stopped, etc.)\n", .{}); - }, - } + const args = [_:null]?[*:0]u8{ + (try arena.allocator().dupeZ(u8, "sh")).ptr, + (try arena.allocator().dupeZ(u8, "-c")).ptr, + (try std.fmt.allocPrintZ(arena.allocator(), "exec {s}", .{cmd})).ptr, + }; + const env_vars = try std.process.createEnvironFromMap(arena.allocator(), env_map, .{}); + if (c.posix_spawnp(&pid, "sh", &actions, &attr, &args, env_vars.ptr) != 0) + return error.PosixSpawnFailed; + // The write end of the pipe is unused by this parent process. + std.posix.close(pipe[1]); + if (set_process_id_opt_ptr_opt) |pid_ptr| + pid_ptr.* = pid; + + const file = std.fs.File{ .handle = pipe[0] }; + const stdout = try file.readToEndAlloc(allocator, 10_000_000); + std.posix.close(pipe[0]);
The file is not closed if readToEndAlloc return an error. Please let me know whether everything is functional and I will send out a v2 addressing this (basically I'm trying to dodge sr.ht's maintenance window).
+ const result = std.posix.waitpid(pid, 0); + // Spawn failure of foreign binaries are reported as successful: + // https://github.com/ziglang/zig/pull/14866 + // TODO: mention this in user documentation. + if (result.status == 0) return stdout; + allocator.free(stdout); + utildbg.log("Early termination of spawned process (via signal, stopped, etc.): {s}\n", .{cmd}); return null; } -- 2.45.2
--- src/api/shellpipe_sync.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/api/shellpipe_sync.zig b/src/api/shellpipe_sync.zig index 9d7517553787..396b4c1c79c0 100644 --- a/src/api/shellpipe_sync.zig +++ b/src/api/shellpipe_sync.zig @@ -48,7 +48,7 @@ fn run_sync_shellpipe(userdata: ?*anyopaque) callconv(.C) c_int { /// for killing the spawn processes. fn kill_sync_shellpipe(mepo: *Mepo) types.Pending { if (mepo.shellpipe_sync_pid) |pid| { - if (std.posix.kill(-pid, std.posix.SIG.KILL)) |_| + if (std.posix.kill(-pid, std.posix.SIG.TERM)) |_| utildbg.log("Killing sync shellpipe process {}\n", .{pid}) else |err| utildbg.log("Error killing sync shellpipe process {}: {}\n", .{ pid, err }); -- 2.45.2
It probably only compiles for targets with std.posix.pid_t anyway so this is only for readability. We should switch back to std.process.Child when both spawn and kill process group lands in Zig. --- src/Mepo.zig | 2 +- src/util/utilprocess.zig | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Mepo.zig b/src/Mepo.zig index 902cc9aa5d16..dccbe67d8dde 100644 --- a/src/Mepo.zig +++ b/src/Mepo.zig @@ -36,7 +36,7 @@ pin_groups: [10]std.ArrayList(types.Pin) = undefined, renderer: *sdl.SDL_Renderer = undefined, renderer_type: types.RendererType = .Auto, shellpipe_async_threads: datastructure.QueueHashMap(i8, sdl.SDL_threadID), -shellpipe_sync_pid: ?std.process.Child.Id = null, +shellpipe_sync_pid: ?std.posix.pid_t = null, table_gestures: std.array_hash_map.AutoArrayHashMap(types.GestureInput, []const u8), table_clicks: std.array_hash_map.AutoArrayHashMap(types.ClickInput, []const u8), table_keybindings: std.array_hash_map.AutoArrayHashMap(types.KeyInput, []const u8), diff --git a/src/util/utilprocess.zig b/src/util/utilprocess.zig index fd05ec37fdf5..0cba5ea2aeef 100644 --- a/src/util/utilprocess.zig +++ b/src/util/utilprocess.zig @@ -11,7 +11,7 @@ pub fn run_subprocess( allocator: std.mem.Allocator, cmd: []const u8, env_map: *const std.process.EnvMap, - set_process_id_opt_ptr_opt: ?*?std.process.Child.Id, + set_process_id_opt_ptr_opt: ?*?std.posix.pid_t, ) !?[]const u8 { var pid: std.c.pid_t = undefined; defer { -- 2.45.2
--- src/api/shellpipe_async.zig | 2 +- src/api/shellpipe_sync.zig | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/api/shellpipe_async.zig b/src/api/shellpipe_async.zig index 216b952db39e..beae56c7db41 100644 --- a/src/api/shellpipe_async.zig +++ b/src/api/shellpipe_async.zig @@ -32,7 +32,7 @@ fn execute(mepo: *Mepo, args: [types.MepoFnNargs]types.MepoArg) !void { fn async_shellpipe(mepo: *Mepo, unique_handle_id: i8, cmd: []const u8) !void { var sp_req = try mepo.allocator.create(AsyncShellpipeRequest); sp_req.unique_handle_id = unique_handle_id; - sp_req.cmd = try mepo.allocator.dupeZ(u8, cmd); + sp_req.cmd = try mepo.allocator.dupe(u8, cmd); sp_req.mepo = mepo; sdl.SDL_DetachThread(sdl.SDL_CreateThread(async_shellpipe_run, "async_shellpipe", sp_req)); } diff --git a/src/api/shellpipe_sync.zig b/src/api/shellpipe_sync.zig index 396b4c1c79c0..ba95898acbed 100644 --- a/src/api/shellpipe_sync.zig +++ b/src/api/shellpipe_sync.zig @@ -21,7 +21,7 @@ fn execute(mepo: *Mepo, args: [types.MepoFnNargs]types.MepoArg) !void { const cmd = args[0].Text; var request = try mepo.allocator.create(Request); request.mepo = mepo; - request.cmd = try mepo.allocator.dupeZ(u8, cmd); + request.cmd = try mepo.allocator.dupe(u8, cmd); for (mepo.uibuttons.items[0..1]) |*button| { if (!button.only_visible_when_sync_shellpipe) unreachable; -- 2.45.2
--- src/api/shellpipe_async.zig | 1 - 1 file changed, 1 deletion(-) diff --git a/src/api/shellpipe_async.zig b/src/api/shellpipe_async.zig index beae56c7db41..2e64acdaaaad 100644 --- a/src/api/shellpipe_async.zig +++ b/src/api/shellpipe_async.zig @@ -74,7 +74,6 @@ pub fn run_shellpipe(mepo: *Mepo, async_id: ?i8, cmd: []const u8) !void { )) |response_stdout| { const heap_stdout = try mepo.allocator.dupeZ(u8, response_stdout); utilsdl.sdl_push_event_jsonapi_execution(heap_stdout); - arena.allocator().free(response_stdout); } } -- 2.45.2