From 679f07605e0e534195138cee7a65699c77ed262c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 18 Mar 2023 18:58:34 -0700 Subject: [PATCH 1/7] termio: detect child process exit --- src/termio/Exec.zig | 71 +++++++++++++++++++++++++++++++++++++++++++++ vendor/libxev | 2 +- 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index 5bd72007e..10d48b8eb 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -105,6 +105,10 @@ pub fn threadEnter(self: *Exec, thread: *termio.Thread) !ThreadData { // Start our subprocess const master_fd = try self.subprocess.start(alloc); errdefer self.subprocess.stop(); + const pid = pid: { + const command = self.subprocess.command orelse return error.ProcessNotStarted; + break :pid command.pid orelse return error.ProcessNoPid; + }; // Setup our data that is used for callbacks var ev_data_ptr = try alloc.create(EventData); @@ -118,6 +122,10 @@ pub fn threadEnter(self: *Exec, thread: *termio.Thread) !ThreadData { var wakeup = try xev.Async.init(); errdefer wakeup.deinit(); + // Watcher to detect subprocess exit + var process = try xev.Process.init(pid); + errdefer process.deinit(); + // Setup our event data before we start ev_data_ptr.* = .{ .writer_mailbox = thread.mailbox, @@ -125,6 +133,7 @@ pub fn threadEnter(self: *Exec, thread: *termio.Thread) !ThreadData { .renderer_state = self.renderer_state, .renderer_wakeup = self.renderer_wakeup, .renderer_mailbox = self.renderer_mailbox, + .process = process, .data_stream = stream, .loop = &thread.loop, .terminal_stream = .{ @@ -143,6 +152,15 @@ pub fn threadEnter(self: *Exec, thread: *termio.Thread) !ThreadData { self.data = ev_data_ptr; errdefer self.data = null; + // Start our process watcher + process.wait( + ev_data_ptr.loop, + &ev_data_ptr.process_wait_c, + EventData, + ev_data_ptr, + processExit, + ); + // Start our reader thread const read_thread = try std.Thread.spawn( .{}, @@ -164,8 +182,12 @@ pub fn threadExit(self: *Exec, data: ThreadData) void { self.data = null; // Stop our subprocess + if (data.ev.process_exited) self.subprocess.externalExit(); self.subprocess.stop(); + // Wait for our subprocess to report it exited. + // data.ev.loop.run(.once); + // Wait for our reader thread to end data.read_thread.join(); } @@ -272,6 +294,14 @@ const EventData = struct { /// The mailbox for notifying the renderer of things. renderer_mailbox: *renderer.Thread.Mailbox, + /// The process watcher + process: xev.Process, + process_exited: bool = false, + + /// This is used for both waiting for the process to exit and then + /// subsequently to wait for the data_stream to close. + process_wait_c: xev.Completion = .{}, + /// The data stream is the main IO for the pty. data_stream: xev.Stream, @@ -301,6 +331,9 @@ const EventData = struct { // Stop our data stream self.data_stream.deinit(); + + // Stop our process watcher + self.process.deinit(); } /// This queues a render operation with the renderer thread. The render @@ -311,6 +344,38 @@ const EventData = struct { } }; +fn processExit( + ev_: ?*EventData, + loop: *xev.Loop, + completion: *xev.Completion, + r: xev.Process.WaitError!u32, +) xev.CallbackAction { + const code = r catch unreachable; + log.debug("child process exited status={}", .{code}); + + const ev = ev_.?; + ev.process_exited = true; + ev.data_stream.close(loop, completion, EventData, ev, ttyClose); + + return .disarm; +} + +fn ttyClose( + _: ?*EventData, + _: *xev.Loop, + _: *xev.Completion, + _: xev.Stream, + r: xev.Stream.CloseError!void, +) xev.CallbackAction { + _ = r catch |err| { + log.warn("error closing tty err={}", .{err}); + return .disarm; + }; + + log.debug("tty parent closed", .{}); + return .disarm; +} + fn ttyWrite( ev_: ?*EventData, _: *xev.Loop, @@ -537,6 +602,12 @@ const Subprocess = struct { return pty.master; } + /// Called to notify that we exited externally so we can unset our + /// running state. + pub fn externalExit(self: *Subprocess) void { + self.command = null; + } + /// Stop the subprocess. This is safe to call anytime. This will wait /// for the subprocess to end so it will block. pub fn stop(self: *Subprocess) void { diff --git a/vendor/libxev b/vendor/libxev index 46e3dafa4..9024a57ef 160000 --- a/vendor/libxev +++ b/vendor/libxev @@ -1 +1 @@ -Subproject commit 46e3dafa477a9a92abe7b425cafdbcbe7ba3574d +Subproject commit 9024a57ef9133382f95927eaf4b6c2b1662598e0 From 872c1211f5d4742c7b63e5815ff60238cdcf3554 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 18 Mar 2023 19:15:29 -0700 Subject: [PATCH 2/7] pty: deinit should close child end --- src/Pty.zig | 1 + src/termio/Exec.zig | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Pty.zig b/src/Pty.zig index f9d419ec0..09a3f9a1a 100644 --- a/src/Pty.zig +++ b/src/Pty.zig @@ -80,6 +80,7 @@ pub fn open(size: winsize) !Pty { pub fn deinit(self: *Pty) void { _ = std.os.system.close(self.master); + _ = std.os.system.close(self.slave); self.* = undefined; } diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index 10d48b8eb..9a2fc7459 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -185,9 +185,6 @@ pub fn threadExit(self: *Exec, data: ThreadData) void { if (data.ev.process_exited) self.subprocess.externalExit(); self.subprocess.stop(); - // Wait for our subprocess to report it exited. - // data.ev.loop.run(.once); - // Wait for our reader thread to end data.read_thread.join(); } From d83bf5aeb4b10bf4ac7cb0ca3c3447e041c31446 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 18 Mar 2023 19:21:28 -0700 Subject: [PATCH 3/7] termio: close surface on process exit --- src/Surface.zig | 16 +++++++++++----- src/apprt/surface.zig | 4 ++++ src/termio/Exec.zig | 17 ++++++++++++----- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index 26903eeed..9d2ee7f62 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -479,6 +479,14 @@ pub fn deinit(self: *Surface) void { log.info("surface closed addr={x}", .{@ptrToInt(self)}); } +/// Close this surface. This will trigger the runtime to start the +/// close process, which should ultimately deinitialize this surface. +pub fn close(self: *Surface) void { + if (@hasDecl(apprt.Surface, "closeSurface")) { + try self.rt_surface.closeSurface(); + } else log.warn("runtime doesn't implement closeSurface", .{}); +} + /// Called from the app thread to handle mailbox messages to our specific /// surface. pub fn handleMessage(self: *Surface, msg: Message) !void { @@ -503,6 +511,8 @@ pub fn handleMessage(self: *Surface, msg: Message) !void { try self.clipboardWrite(v.data); }, }, + + .close => self.close(), } } @@ -953,11 +963,7 @@ pub fn keyCallback( } else log.warn("runtime doesn't implement gotoSplit", .{}); }, - .close_surface => { - if (@hasDecl(apprt.Surface, "closeSurface")) { - try self.rt_surface.closeSurface(); - } else log.warn("runtime doesn't implement closeSurface", .{}); - }, + .close_surface => self.close(), .close_window => { _ = self.app_mailbox.push(.{ .close = self }, .{ .instant = {} }); diff --git a/src/apprt/surface.zig b/src/apprt/surface.zig index 0e6439a41..3f409efb7 100644 --- a/src/apprt/surface.zig +++ b/src/apprt/surface.zig @@ -23,6 +23,10 @@ pub const Message = union(enum) { /// Write the clipboard contents. clipboard_write: WriteReq, + + /// Close the surface. This will only close the current surface that + /// receives this, not the full application. + close: void, }; /// A surface mailbox. diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index 9a2fc7459..9078d77de 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -130,6 +130,7 @@ pub fn threadEnter(self: *Exec, thread: *termio.Thread) !ThreadData { ev_data_ptr.* = .{ .writer_mailbox = thread.mailbox, .writer_wakeup = thread.wakeup, + .surface_mailbox = self.surface_mailbox, .renderer_state = self.renderer_state, .renderer_wakeup = self.renderer_wakeup, .renderer_mailbox = self.renderer_mailbox, @@ -142,7 +143,6 @@ pub fn threadEnter(self: *Exec, thread: *termio.Thread) !ThreadData { .ev = ev_data_ptr, .terminal = &self.terminal, .grid_size = &self.grid_size, - .surface_mailbox = self.surface_mailbox, }, }, }; @@ -277,6 +277,9 @@ const EventData = struct { writer_mailbox: *termio.Mailbox, writer_wakeup: xev.Async, + /// Mailbox for the surface. + surface_mailbox: apprt.surface.Mailbox, + /// The stream parser. This parses the stream of escape codes and so on /// from the child process and calls callbacks in the stream handler. terminal_stream: terminal.Stream(StreamHandler), @@ -354,6 +357,11 @@ fn processExit( ev.process_exited = true; ev.data_stream.close(loop, completion, EventData, ev, ttyClose); + // Notify our surface we want to close + _ = ev.surface_mailbox.push(.{ + .close = {}, + }, .{ .forever = {} }); + return .disarm; } @@ -820,7 +828,6 @@ const StreamHandler = struct { alloc: Allocator, grid_size: *renderer.GridSize, terminal: *terminal.Terminal, - surface_mailbox: apprt.surface.Mailbox, /// This is set to true when a message was written to the writer /// mailbox. This can be used by callers to determine if they need @@ -1167,7 +1174,7 @@ const StreamHandler = struct { std.mem.copy(u8, &buf, title); buf[title.len] = 0; - _ = self.surface_mailbox.push(.{ + _ = self.ev.surface_mailbox.push(.{ .set_title = buf, }, .{ .forever = {} }); } @@ -1179,14 +1186,14 @@ const StreamHandler = struct { // Get clipboard contents if (data.len == 1 and data[0] == '?') { - _ = self.surface_mailbox.push(.{ + _ = self.ev.surface_mailbox.push(.{ .clipboard_read = kind, }, .{ .forever = {} }); return; } // Write clipboard contents - _ = self.surface_mailbox.push(.{ + _ = self.ev.surface_mailbox.push(.{ .clipboard_write = try apprt.surface.Message.WriteReq.init( self.alloc, data, From 00c837e0d2a48545803d5b2729af718c36d3a3f8 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 18 Mar 2023 19:25:54 -0700 Subject: [PATCH 4/7] apprt: all implement close surface --- src/Surface.zig | 6 +++--- src/apprt/embedded.zig | 2 +- src/apprt/glfw.zig | 5 +++++ src/apprt/gtk.zig | 2 +- 4 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index 9d2ee7f62..d83926b43 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -482,9 +482,9 @@ pub fn deinit(self: *Surface) void { /// Close this surface. This will trigger the runtime to start the /// close process, which should ultimately deinitialize this surface. pub fn close(self: *Surface) void { - if (@hasDecl(apprt.Surface, "closeSurface")) { - try self.rt_surface.closeSurface(); - } else log.warn("runtime doesn't implement closeSurface", .{}); + if (@hasDecl(apprt.Surface, "close")) { + self.rt_surface.close(); + } else log.warn("runtime doesn't implement close", .{}); } /// Called from the app thread to handle mailbox messages to our specific diff --git a/src/apprt/embedded.zig b/src/apprt/embedded.zig index 063743b44..8d12085a8 100644 --- a/src/apprt/embedded.zig +++ b/src/apprt/embedded.zig @@ -167,7 +167,7 @@ pub const Surface = struct { func(self.opts.userdata, direction); } - pub fn closeSurface(self: *const Surface) !void { + pub fn close(self: *const Surface) void { const func = self.app.opts.close_surface orelse { log.info("runtime embedder does not support closing a surface", .{}); return; diff --git a/src/apprt/glfw.zig b/src/apprt/glfw.zig index 533887349..1995f03cd 100644 --- a/src/apprt/glfw.zig +++ b/src/apprt/glfw.zig @@ -378,6 +378,11 @@ pub const Surface = struct { try self.app.newTab(&self.core_surface); } + /// Close this surface. + pub fn close(self: *const Surface) void { + self.window.setShouldClose(true); + } + /// Set the size limits of the window. /// Note: this interface is not good, we should redo it if we plan /// to use this more. i.e. you can't set max width but no max height, diff --git a/src/apprt/gtk.zig b/src/apprt/gtk.zig index da69ed3e4..928947612 100644 --- a/src/apprt/gtk.zig +++ b/src/apprt/gtk.zig @@ -614,7 +614,7 @@ pub const Surface = struct { } /// Close this surface. - fn close(self: *Surface) void { + pub fn close(self: *Surface) void { self.window.closeSurface(self); } From 6b61a2449a0c71a00e3d3686350e34cf187fdb4e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 18 Mar 2023 19:30:01 -0700 Subject: [PATCH 5/7] termio: no need to close pty primary when process exits --- src/termio/Exec.zig | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index 9078d77de..37150a5cf 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -346,8 +346,8 @@ const EventData = struct { fn processExit( ev_: ?*EventData, - loop: *xev.Loop, - completion: *xev.Completion, + _: *xev.Loop, + _: *xev.Completion, r: xev.Process.WaitError!u32, ) xev.CallbackAction { const code = r catch unreachable; @@ -355,7 +355,6 @@ fn processExit( const ev = ev_.?; ev.process_exited = true; - ev.data_stream.close(loop, completion, EventData, ev, ttyClose); // Notify our surface we want to close _ = ev.surface_mailbox.push(.{ From 91c9655475ced0dd8079da4abd35e1859f624b8a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 18 Mar 2023 19:33:08 -0700 Subject: [PATCH 6/7] remove unused function, make apprt surface close mandatory --- src/Surface.zig | 4 +--- src/termio/Exec.zig | 16 ---------------- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index d83926b43..d29aead0c 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -482,9 +482,7 @@ pub fn deinit(self: *Surface) void { /// Close this surface. This will trigger the runtime to start the /// close process, which should ultimately deinitialize this surface. pub fn close(self: *Surface) void { - if (@hasDecl(apprt.Surface, "close")) { - self.rt_surface.close(); - } else log.warn("runtime doesn't implement close", .{}); + self.rt_surface.close(); } /// Called from the app thread to handle mailbox messages to our specific diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index 37150a5cf..c1a903b7f 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -364,22 +364,6 @@ fn processExit( return .disarm; } -fn ttyClose( - _: ?*EventData, - _: *xev.Loop, - _: *xev.Completion, - _: xev.Stream, - r: xev.Stream.CloseError!void, -) xev.CallbackAction { - _ = r catch |err| { - log.warn("error closing tty err={}", .{err}); - return .disarm; - }; - - log.debug("tty parent closed", .{}); - return .disarm; -} - fn ttyWrite( ev_: ?*EventData, _: *xev.Loop, From ddbc0dc58681c96da5e127c749f549ea4df5c7b0 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 18 Mar 2023 19:40:42 -0700 Subject: [PATCH 7/7] apprt/embedded: incorrect function call for new close surface --- src/apprt/embedded.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/apprt/embedded.zig b/src/apprt/embedded.zig index 8d12085a8..b782ba8cf 100644 --- a/src/apprt/embedded.zig +++ b/src/apprt/embedded.zig @@ -486,7 +486,7 @@ pub const CAPI = struct { /// Request that the surface become closed. This will go through the /// normal trigger process that a close surface input binding would. export fn ghostty_surface_request_close(ptr: *Surface) void { - ptr.closeSurface() catch {}; + ptr.close(); } /// Request that the surface split in the given direction.