This thread contains a patchset. You're looking at the original emails,
but you may wish to use the patch review UI.
Review patch
6
2
[PATCH mepo 1/5] Rework run_process to base on posix_spawn
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(-)
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;
+ 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]);
+ 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
[PATCH mepo 2/5] Use the proper signal SIGTERM to kill shellpipe
---
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
[PATCH mepo 3/5] Use more explicit types for PID in the API
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
[PATCH mepo 4/5] Match sentinality with API
---
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
[PATCH mepo 5/5] Remove no-op free by arena allocator
---
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
On 2024-08-05 at 16:18+09:00, Nguyễn Gia Phong wrote:
> + 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:
> + 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).
On Mon, Aug 5, 2024, at 3:18 AM, Nguyễn Gia Phong wrote:
> 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.