From 3fae05e2dc916c86b6d5fb990ea1e43c6f28f0be Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 30 Dec 2023 15:35:35 -0800 Subject: [PATCH 1/7] termio/exec: detect abnormally short runtime and show an error message --- src/termio/Exec.zig | 71 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 2 deletions(-) diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index 4c204f792..0323919fc 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -38,6 +38,11 @@ const c = @cImport({ /// correct granularity of events. const disable_kitty_keyboard_protocol = apprt.runtime == apprt.glfw; +/// The number of milliseconds below which we consider a process +/// exit to be abnormal. This is used to show an error message +/// when the process exits too quickly. +const abnormal_runtime_threshold_ms = 250; + /// Allocator alloc: Allocator, @@ -230,13 +235,16 @@ pub fn threadEnter(self: *Exec, thread: *termio.Thread) !ThreadData { self.execFailedInChild() catch {}; std.os.exit(1); }; - errdefer self.subprocess.stop(); const pid = pid: { const command = self.subprocess.command orelse return error.ProcessNotStarted; break :pid command.pid orelse return error.ProcessNoPid; }; + // Track our process start time so we know how long it was + // running for. + const process_start = try std.time.Instant.now(); + // Create our pipe that we'll use to kill our read thread. // pipe[0] is the read end, pipe[1] is the write end. const pipe = try internal_os.pipe(); @@ -268,6 +276,7 @@ pub fn threadEnter(self: *Exec, thread: *termio.Thread) !ThreadData { .renderer_wakeup = self.renderer_wakeup, .renderer_mailbox = self.renderer_mailbox, .process = process, + .process_start = process_start, .data_stream = stream, .loop = &thread.loop, .terminal_stream = .{ @@ -538,6 +547,15 @@ pub fn jumpToPrompt(self: *Exec, delta: isize) !void { pub inline fn queueWrite(self: *Exec, data: []const u8, linefeed: bool) !void { const ev = self.data.?; + // If our process is exited then we send our surface a message + // about it but we don't queue any more writes. + if (ev.process_exited) { + _ = ev.surface_mailbox.push(.{ + .child_exited = {}, + }, .{ .forever = {} }); + return; + } + // We go through and chunk the data if necessary to fit into // our cached buffers that we can queue to the stream. var i: usize = 0; @@ -640,6 +658,7 @@ const EventData = struct { /// The process watcher process: xev.Process, + process_start: std.time.Instant, process_exited: bool = false, /// This is used for both waiting for the process to exit and then @@ -703,11 +722,59 @@ fn processExit( 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; + // Determine how long the process was running for. + const runtime_ms: ?u64 = runtime: { + const process_end = std.time.Instant.now() catch break :runtime null; + const runtime_ns = process_end.since(ev.process_start); + const runtime_ms = runtime_ns / std.time.ns_per_ms; + break :runtime runtime_ms; + }; + log.debug( + "child process exited status={} runtime={}ms", + .{ code, runtime_ms orelse 0 }, + ); + + // If our runtime was below some threshold then we assume that this + // was an abnormal exit and we show an error message. + if (runtime_ms) |runtime| runtime: { + if (runtime > abnormal_runtime_threshold_ms) break :runtime; + log.warn("abnormal process exit detected, showing error message", .{}); + + // Modify the terminal to show our error message. This + // requires grabbing the renderer state lock. + ev.renderer_state.mutex.lock(); + defer ev.renderer_state.mutex.unlock(); + const alloc = ev.terminal_stream.handler.alloc; + const t = ev.renderer_state.terminal; + + // Reset the terminal completely. + t.fullReset(alloc); + + // Write our message out. + const view = std.unicode.Utf8View.init( + \\ Ghostty failed to launch the requested command. + \\ Please check your "command" configuration. + \\ + \\ Press any key to exit. + ) catch break :runtime; + var it = view.iterator(); + while (it.nextCodepoint()) |cp| { + if (cp == '\n') { + t.carriageReturn(); + t.linefeed() catch break :runtime; + continue; + } + + t.print(cp) catch break :runtime; + } + + return .disarm; + } + // Notify our surface we want to close _ = ev.surface_mailbox.push(.{ .child_exited = {}, From 3ee842e1b710b0f6ac15b8702ed189e9fc77111b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 30 Dec 2023 15:36:13 -0800 Subject: [PATCH 2/7] config: remove command validation Abnormal exit detection is more robust, and this validation always had issues for example we didn't parse shell escapes and so on. --- src/config/Config.zig | 47 ------------------------------------------- 1 file changed, 47 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index 772458b57..ccdd51429 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -1707,53 +1707,6 @@ pub fn finalize(self: *Config) !void { } } - if (self.command) |command| command: { - // If the command has a space in it, we skip the check below because - // its probably a multi-argument command. These types of commands - // can contain full shell escapes and other characters and we don't - // want to reimplement that all here. The point of the check below - // is MOSTLY to find people who do `command = myshell` where "myshell" - // is installed by something like Homebrew and therefore isn't available - // to a login shell. We will do more robust checks in the future by - // simply checking if the command exited with an error. - if (std.mem.indexOf(u8, command, " ") != null) break :command; - - // If the path is not absolute then we want to expand it. We use our - // current path because our current path is what will be available - // to our termio launcher. - if (!std.fs.path.isAbsolute(command)) { - const expanded = Command.expandPath(alloc, command) catch |err| expanded: { - log.warn("failed to expand command path={s} err={}", .{ command, err }); - break :expanded null; - }; - - if (expanded) |v| { - self.command = v; - } else { - // If the command is not found on the path, we put an error - // but we still allow the command to remain unchanged and try - // to launch it later. - try self._errors.add(alloc, .{ - .message = try std.fmt.allocPrintZ( - alloc, - "command {s}: not found on PATH, use an absolute path", - .{command}, - ), - }); - } - } else { - std.fs.accessAbsolute(command, .{}) catch { - try self._errors.add(alloc, .{ - .message = try std.fmt.allocPrintZ( - alloc, - "command {s}: file not found", - .{command}, - ), - }); - }; - } - } - // If we have the special value "inherit" then set it to null which // does the same. In the future we should change to a tagged union. if (std.mem.eql(u8, wd, "inherit")) self.@"working-directory" = null; From fc963064c6400d86d22973bb0741d9ed2ad57553 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 30 Dec 2023 15:43:50 -0800 Subject: [PATCH 3/7] termio/exec: abnormal exit can use exit code on linux --- src/termio/Exec.zig | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index 0323919fc..ff4ed2ac8 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -741,6 +741,14 @@ fn processExit( // If our runtime was below some threshold then we assume that this // was an abnormal exit and we show an error message. if (runtime_ms) |runtime| runtime: { + // On macOS, our exit code detection doesn't work, possibly + // because of our `login` wrapper. More investigation required. + if (comptime !builtin.target.isDarwin()) { + // If our exit code is zero, then the command was successful + // and we don't ever consider it abnormal. + if (code == 0) break :runtime; + } + if (runtime > abnormal_runtime_threshold_ms) break :runtime; log.warn("abnormal process exit detected, showing error message", .{}); From aaded1f31124c8d636a4f8dfe8b59a8ac5ab3e4c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 30 Dec 2023 15:52:26 -0800 Subject: [PATCH 4/7] termio/exec: use arraylist to build up message for error --- src/termio/Exec.zig | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index ff4ed2ac8..7d9e47aee 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -749,26 +749,38 @@ fn processExit( if (code == 0) break :runtime; } + // Our runtime always has to be under the threshold to be + // considered abnormal. This is because a user can always + // manually do something like `exit 1` in their shell to + // force the exit code to be non-zero. We only want to detect + // abnormal exits that happen so quickly the user can't react. if (runtime > abnormal_runtime_threshold_ms) break :runtime; log.warn("abnormal process exit detected, showing error message", .{}); + // Build our error message. Do this outside of the renderer lock. + const alloc = ev.terminal_stream.handler.alloc; + var msg = std.ArrayList(u8).init(alloc); + defer msg.deinit(); + var writer = msg.writer(); + writer.writeAll( + \\ Ghostty failed to launch the requested command. + \\ Please check your "command" configuration. + \\ + \\ Press any key to close this window. + ) catch break :runtime; + // Modify the terminal to show our error message. This // requires grabbing the renderer state lock. ev.renderer_state.mutex.lock(); defer ev.renderer_state.mutex.unlock(); - const alloc = ev.terminal_stream.handler.alloc; const t = ev.renderer_state.terminal; // Reset the terminal completely. t.fullReset(alloc); // Write our message out. - const view = std.unicode.Utf8View.init( - \\ Ghostty failed to launch the requested command. - \\ Please check your "command" configuration. - \\ - \\ Press any key to exit. - ) catch break :runtime; + const view = std.unicode.Utf8View.init(msg.items) catch + break :runtime; var it = view.iterator(); while (it.nextCodepoint()) |cp| { if (cp == '\n') { From f3aaa884c60c440f06f7886416c1eb7f249aca7e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 30 Dec 2023 17:51:34 -0800 Subject: [PATCH 5/7] termio/exec: use message to writer thread so we can output failed cmd --- src/termio/Exec.zig | 90 ++++++++++++++++++++++++++---------------- src/termio/Thread.zig | 1 + src/termio/message.zig | 6 +++ 3 files changed, 63 insertions(+), 34 deletions(-) diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index 7d9e47aee..d4579e4c2 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -544,6 +544,56 @@ pub fn jumpToPrompt(self: *Exec, delta: isize) !void { } } +/// Called when the child process exited abnormally but before +/// the surface is notified. +pub fn childExitedAbnormally(self: *Exec) !void { + // Build up our command for the error message + const command = try std.mem.join( + self.alloc, + " ", + self.subprocess.args, + ); + defer self.alloc.free(command); + + // Build our error message. Do this outside of the renderer lock. + var msg = std.ArrayList(u8).init(self.alloc); + defer msg.deinit(); + var writer = msg.writer(); + try writer.print( + \\Ghostty failed to launch the requested command. + \\Please check your "command" configuration. + \\ + \\Command: {s} + \\ + \\Press any key to close this window. + , .{command}); + + // Modify the terminal to show our error message. This + // requires grabbing the renderer state lock. + self.renderer_state.mutex.lock(); + defer self.renderer_state.mutex.unlock(); + const t = self.renderer_state.terminal; + + // Reset the terminal completely. + // NOTE: The error output is in the terminal at this point. In the + // future, we can make an even better error message by scrolling, + // writing at the bottom, etc. + t.fullReset(self.alloc); + + // Write our message out. + const view = try std.unicode.Utf8View.init(msg.items); + var it = view.iterator(); + while (it.nextCodepoint()) |cp| { + if (cp == '\n') { + t.carriageReturn(); + try t.linefeed(); + continue; + } + + try t.print(cp); + } +} + pub inline fn queueWrite(self: *Exec, data: []const u8, linefeed: bool) !void { const ev = self.data.?; @@ -757,40 +807,12 @@ fn processExit( if (runtime > abnormal_runtime_threshold_ms) break :runtime; log.warn("abnormal process exit detected, showing error message", .{}); - // Build our error message. Do this outside of the renderer lock. - const alloc = ev.terminal_stream.handler.alloc; - var msg = std.ArrayList(u8).init(alloc); - defer msg.deinit(); - var writer = msg.writer(); - writer.writeAll( - \\ Ghostty failed to launch the requested command. - \\ Please check your "command" configuration. - \\ - \\ Press any key to close this window. - ) catch break :runtime; - - // Modify the terminal to show our error message. This - // requires grabbing the renderer state lock. - ev.renderer_state.mutex.lock(); - defer ev.renderer_state.mutex.unlock(); - const t = ev.renderer_state.terminal; - - // Reset the terminal completely. - t.fullReset(alloc); - - // Write our message out. - const view = std.unicode.Utf8View.init(msg.items) catch - break :runtime; - var it = view.iterator(); - while (it.nextCodepoint()) |cp| { - if (cp == '\n') { - t.carriageReturn(); - t.linefeed() catch break :runtime; - continue; - } - - t.print(cp) catch break :runtime; - } + // Notify our main writer thread which has access to more + // information so it can show a better error message. + _ = ev.writer_mailbox.push(.{ + .child_exited_abnormally = {}, + }, .{ .forever = {} }); + ev.writer_wakeup.notify() catch break :runtime; return .disarm; } diff --git a/src/termio/Thread.zig b/src/termio/Thread.zig index 93faa38d5..516102f3f 100644 --- a/src/termio/Thread.zig +++ b/src/termio/Thread.zig @@ -186,6 +186,7 @@ fn drainMailbox(self: *Thread) !void { .jump_to_prompt => |v| try self.impl.jumpToPrompt(v), .start_synchronized_output => self.startSynchronizedOutput(), .linefeed_mode => |v| self.flags.linefeed_mode = v, + .child_exited_abnormally => try self.impl.childExitedAbnormally(), .write_small => |v| try self.impl.queueWrite(v.data[0..v.len], self.flags.linefeed_mode), .write_stable => |v| try self.impl.queueWrite(v, self.flags.linefeed_mode), .write_alloc => |v| { diff --git a/src/termio/message.zig b/src/termio/message.zig index 955fa44ed..223e09c10 100644 --- a/src/termio/message.zig +++ b/src/termio/message.zig @@ -62,6 +62,12 @@ pub const Message = union(enum) { /// Enable or disable linefeed mode (mode 20). linefeed_mode: bool, + /// The child exited abnormally. The termio state is marked + /// as process exited but the surface hasn't been notified to + /// close because termio can use this to update the terminal + /// with an error message. + child_exited_abnormally: void, + /// Write where the data fits in the union. write_small: WriteReq.Small, From 53dffc8e18dd3500bb60ef431bc3890f254283be Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 30 Dec 2023 19:34:45 -0800 Subject: [PATCH 6/7] termio/exec: style the exec failure error better --- src/terminal/Terminal.zig | 6 ++-- src/termio/Exec.zig | 62 +++++++++++++++++++++------------------ 2 files changed, 36 insertions(+), 32 deletions(-) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 32f6ced86..af58f75da 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -425,7 +425,7 @@ fn clearPromptForResize(self: *Terminal) void { /// encoded as "\n". This omits any formatting such as fg/bg. /// /// The caller must free the string. -fn plainString(self: *Terminal, alloc: Allocator) ![]const u8 { +pub fn plainString(self: *Terminal, alloc: Allocator) ![]const u8 { return try self.screen.testString(alloc, .viewport); } @@ -715,9 +715,7 @@ pub fn invokeCharset( /// Print UTF-8 encoded string to the terminal. This string must be /// a single line, newlines and carriage returns and other control /// characters are not processed. -/// -/// This is not public because it is only used for tests rigt now. -fn printString(self: *Terminal, str: []const u8) !void { +pub fn printString(self: *Terminal, str: []const u8) !void { const view = try std.unicode.Utf8View.init(str); var it = view.iterator(); while (it.nextCodepoint()) |cp| try self.print(cp); diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index d4579e4c2..c5d66decd 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -555,43 +555,49 @@ pub fn childExitedAbnormally(self: *Exec) !void { ); defer self.alloc.free(command); - // Build our error message. Do this outside of the renderer lock. - var msg = std.ArrayList(u8).init(self.alloc); - defer msg.deinit(); - var writer = msg.writer(); - try writer.print( - \\Ghostty failed to launch the requested command. - \\Please check your "command" configuration. - \\ - \\Command: {s} - \\ - \\Press any key to close this window. - , .{command}); - // Modify the terminal to show our error message. This // requires grabbing the renderer state lock. self.renderer_state.mutex.lock(); defer self.renderer_state.mutex.unlock(); const t = self.renderer_state.terminal; - // Reset the terminal completely. - // NOTE: The error output is in the terminal at this point. In the - // future, we can make an even better error message by scrolling, - // writing at the bottom, etc. - t.fullReset(self.alloc); + // No matter what move the cursor back to the column 0. + t.carriageReturn(); - // Write our message out. - const view = try std.unicode.Utf8View.init(msg.items); - var it = view.iterator(); - while (it.nextCodepoint()) |cp| { - if (cp == '\n') { - t.carriageReturn(); - try t.linefeed(); - continue; - } + // Reset styles + try t.setAttribute(.{ .unset = {} }); - try t.print(cp); + // If there is data in the viewport, we want to scroll down + // a little bit and write a horizontal rule before writing + // our message. This lets the use see the error message the + // command may have output. + const viewport_str = try t.plainString(self.alloc); + defer self.alloc.free(viewport_str); + if (viewport_str.len > 0) { + try t.linefeed(); + for (0..t.cols) |_| try t.print(0x2501); + t.carriageReturn(); + try t.linefeed(); + try t.linefeed(); } + + // Output our error message + try t.setAttribute(.{ .@"8_fg" = .bright_red }); + try t.setAttribute(.{ .bold = {} }); + try t.printString("Ghostty failed to launch the requested command:"); + try t.setAttribute(.{ .unset = {} }); + try t.setAttribute(.{ .faint = {} }); + t.carriageReturn(); + try t.linefeed(); + try t.printString(command); + try t.setAttribute(.{ .unset = {} }); + t.carriageReturn(); + try t.linefeed(); + try t.linefeed(); + try t.printString("Press any key to close the window."); + + // Hide the cursor + t.modes.set(.cursor_visible, false); } pub inline fn queueWrite(self: *Exec, data: []const u8, linefeed: bool) !void { From 730343c600c4090410a7354d611d0f2bcda7e5c2 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sat, 30 Dec 2023 19:37:38 -0800 Subject: [PATCH 7/7] termio/exec: pass code and runtime to error but don't show it yet --- src/termio/Exec.zig | 5 ++++- src/termio/message.zig | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index c5d66decd..ff7841647 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -816,7 +816,10 @@ fn processExit( // Notify our main writer thread which has access to more // information so it can show a better error message. _ = ev.writer_mailbox.push(.{ - .child_exited_abnormally = {}, + .child_exited_abnormally = .{ + .code = code, + .runtime_ms = runtime, + }, }, .{ .forever = {} }); ev.writer_wakeup.notify() catch break :runtime; diff --git a/src/termio/message.zig b/src/termio/message.zig index 223e09c10..e6c8fc76f 100644 --- a/src/termio/message.zig +++ b/src/termio/message.zig @@ -66,7 +66,10 @@ pub const Message = union(enum) { /// as process exited but the surface hasn't been notified to /// close because termio can use this to update the terminal /// with an error message. - child_exited_abnormally: void, + child_exited_abnormally: struct { + code: u32, + runtime_ms: u64, + }, /// Write where the data fits in the union. write_small: WriteReq.Small,