From 2dba5abcb2ee8d568b8f0181223930b598ea3c90 Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Tue, 10 Jun 2025 14:10:12 +0800 Subject: [PATCH] fix(tui): wait for embedded server's exit code Uses the undocumented "error_exit" UI event for a different purpose: When :detach is used on the server, send an "error_exit" with 0 `status` to indicate that the server shouldn't wait for client exit. --- src/nvim/api/ui.c | 23 ++++++++++++------ src/nvim/api/ui_events.in.h | 10 +++++++- src/nvim/event/proc.c | 20 +++++++++++---- src/nvim/ex_docmd.c | 6 ++--- src/nvim/msgpack_rpc/channel.c | 11 ++++++--- src/nvim/tui/tui.c | 22 ++++++----------- src/nvim/ui_client.c | 10 ++++++++ src/nvim/ui_client.h | 6 ++++- test/functional/terminal/tui_spec.lua | 35 ++++++++++++++++++++++++--- 9 files changed, 104 insertions(+), 39 deletions(-) diff --git a/src/nvim/api/ui.c b/src/nvim/api/ui.c index 27c8acb40c..0e12951575 100644 --- a/src/nvim/api/ui.c +++ b/src/nvim/api/ui.c @@ -73,12 +73,26 @@ static void remote_ui_destroy(RemoteUI *ui) xfree(ui); } -void remote_ui_disconnect(uint64_t channel_id) +/// Removes the client on the given channel from the list of UIs. +/// +/// @param err if non-NULL and there is no UI on the channel, set an error +/// @param send_error_exit send an "error_exit" event with 0 status first +void remote_ui_disconnect(uint64_t channel_id, Error *err, bool send_error_exit) { RemoteUI *ui = pmap_get(uint64_t)(&connected_uis, channel_id); if (!ui) { + if (err != NULL) { + api_set_error(err, kErrorTypeException, + "UI not attached to channel: %" PRId64, channel_id); + } return; } + if (send_error_exit) { + MAXSIZE_TEMP_ARRAY(args, 1); + ADD_C(args, INTEGER_OBJ(0)); + push_call(ui, "error_exit", args); + ui_flush_buf(ui); + } pmap_del(uint64_t)(&connected_uis, channel_id, NULL); ui_detach_impl(ui, channel_id); remote_ui_destroy(ui); @@ -233,12 +247,7 @@ void nvim_ui_set_focus(uint64_t channel_id, Boolean gained, Error *error) void nvim_ui_detach(uint64_t channel_id, Error *err) FUNC_API_SINCE(1) FUNC_API_REMOTE_ONLY { - if (!map_has(uint64_t, &connected_uis, channel_id)) { - api_set_error(err, kErrorTypeException, - "UI not attached to channel: %" PRId64, channel_id); - return; - } - remote_ui_disconnect(channel_id); + remote_ui_disconnect(channel_id, err, false); } /// Sends a "restart" UI event to the UI on the given channel. diff --git a/src/nvim/api/ui_events.in.h b/src/nvim/api/ui_events.in.h index d060850958..f97679a5a4 100644 --- a/src/nvim/api/ui_events.in.h +++ b/src/nvim/api/ui_events.in.h @@ -177,5 +177,13 @@ void msg_history_show(Array entries) void msg_history_clear(void) FUNC_API_SINCE(10) FUNC_API_REMOTE_ONLY; +// This UI event is currently undocumented. +// - When the server needs to intentionally exit with an exit code, and there is no +// message in server stderr for the user, this event is sent with positive `status` +// argument, to indicate that the UI should exit normally with `status`. +// - When the server has crashed or there is a message in server stderr for the user, +// this event is not sent, and the UI should make server stderr visible. +// - When :detach is used on the server, this event is sent with a zero `status` +// argument, to indicate that the UI shouldn't wait for server exit. void error_exit(Integer status) - FUNC_API_SINCE(12); + FUNC_API_SINCE(12) FUNC_API_CLIENT_IMPL; diff --git a/src/nvim/event/proc.c b/src/nvim/event/proc.c index e02f9eb6cd..48096e26f3 100644 --- a/src/nvim/event/proc.c +++ b/src/nvim/event/proc.c @@ -5,6 +5,7 @@ #include #include "klib/kvec.h" +#include "nvim/channel.h" #include "nvim/event/libuv_proc.h" #include "nvim/event/loop.h" #include "nvim/event/multiqueue.h" @@ -437,16 +438,25 @@ static void on_proc_exit(Proc *proc) Loop *loop = proc->loop; ILOG("child exited: pid=%d status=%d" PRIu64, proc->pid, proc->status); -#ifdef MSWIN - // XXX: This assumes the TUI never spawns any other processes...? - // TODO(justinmk): figure out why rpc_close sometimes(??) isn't called, then remove this jank. + // TODO(justinmk): figure out why rpc_close sometimes(??) isn't called. // Theories: // - EOF not received in receive_msgpack, then doesn't call chan_close_on_err(). // - proc_close_handles not tickled by ui_client.c's LOOP_PROCESS_EVENTS? if (ui_client_channel_id) { - exit_on_closed_chan(proc->status); + uint64_t server_chan_id = ui_client_channel_id; + Channel *server_chan = find_channel(server_chan_id); + if (server_chan != NULL && server_chan->streamtype == kChannelStreamProc + && proc == &server_chan->stream.proc) { + // Need to call ui_client_may_restart_server() here as well, as sometimes + // rpc_close_event() hasn't been called yet (also see comments above). + ui_client_may_restart_server(); + if (ui_client_channel_id == server_chan_id) { + // If the current embedded server has exited and no new server is started, + // the client should exit with the same status. + exit_on_closed_chan(proc->status); + } + } } -#endif // Process has terminated, but there could still be data to be read from the // OS. We are still in the libuv loop, so we cannot call code that polls for diff --git a/src/nvim/ex_docmd.c b/src/nvim/ex_docmd.c index eb61195ef1..0bdf19300d 100644 --- a/src/nvim/ex_docmd.c +++ b/src/nvim/ex_docmd.c @@ -5560,8 +5560,8 @@ static void ex_detach(exarg_T *eap) if (eap && eap->forceit) { emsg("bang (!) not supported yet"); } else { - // 1. (TODO) Send "detach" UI-event (notification only). - // 2. Perform server-side `nvim_ui_detach`. + // 1. Send "error_exit" UI-event (notification only). + // 2. Perform server-side UI detach. // 3. Close server-side channel without self-exit. if (!current_ui) { @@ -5578,7 +5578,7 @@ static void ex_detach(exarg_T *eap) // Server-side UI detach. Doesn't close the channel. Error err2 = ERROR_INIT; - nvim_ui_detach(chan->id, &err2); + remote_ui_disconnect(chan->id, &err2, true); if (ERROR_SET(&err2)) { emsg(err2.msg); // UI disappeared already? api_clear_error(&err2); diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index 1f8513baef..d543c3064e 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -498,7 +498,7 @@ static void rpc_close_event(void **argv) if (is_ui_client || channel->streamtype == kChannelStreamStdio) { if (!is_ui_client) { // Avoid hanging when there are no other UIs and a prompt is triggered on exit. - remote_ui_disconnect(channel->id); + remote_ui_disconnect(channel->id, NULL, false); } else { ui_client_may_restart_server(); if (ui_client_channel_id != channel->id) { @@ -507,14 +507,19 @@ static void rpc_close_event(void **argv) } } if (!channel->detach) { - exit_on_closed_chan(channel->exit_status == -1 ? 0 : channel->exit_status); + if (channel->streamtype == kChannelStreamProc && ui_client_error_exit < 0) { + // Wait for the embedded server to exit instead of exiting immediately, + // as it's necessary to get the server's exit code in on_proc_exit(). + } else { + exit_on_closed_chan(0); + } } } } void rpc_free(Channel *channel) { - remote_ui_disconnect(channel->id); + remote_ui_disconnect(channel->id, NULL, false); unpacker_teardown(channel->rpc.unpacker); xfree(channel->rpc.unpacker); diff --git a/src/nvim/tui/tui.c b/src/nvim/tui/tui.c index dde53ec0e3..aae490378a 100644 --- a/src/nvim/tui/tui.c +++ b/src/nvim/tui/tui.c @@ -142,7 +142,6 @@ struct TUIData { char *space_buf; size_t space_buf_len; bool stopped; - int seen_error_exit; int width; int height; bool rgb; @@ -165,7 +164,6 @@ void tui_start(TUIData **tui_p, int *width, int *height, char **term, bool *rgb) tui->is_starting = true; tui->screenshot = NULL; tui->stopped = false; - tui->seen_error_exit = 0; tui->loop = &main_loop; tui->url = -1; @@ -568,14 +566,16 @@ static void terminfo_disable(TUIData *tui) /// Disable the alternate screen and prepare for the TUI to close. static void terminfo_stop(TUIData *tui) { - if (ui_client_exit_status == 0) { - ui_client_exit_status = tui->seen_error_exit; + if (ui_client_exit_status == 0 && ui_client_error_exit > 0) { + ui_client_exit_status = ui_client_error_exit; } - // if nvim exited with nonzero status, without indicated this was an + // If Nvim exited with nonzero status, without indicating this was an // intentional exit (like `:1cquit`), it likely was an internal failure. - // Don't clobber the stderr error message in this case. - if (ui_client_exit_status == tui->seen_error_exit) { + // Don't clobber the stderr error message in this case. #21608 + if (ui_client_exit_status == MAX(ui_client_error_exit, 0)) { + // Position the cursor on the last screen line, below all the text + cursor_goto(tui, tui->height - 1, 0); // Exit alternate screen. unibi_out(tui, unibi_exit_ca_mode); } @@ -614,12 +614,6 @@ static void tui_terminal_after_startup(TUIData *tui) flush_buf(tui); } -void tui_error_exit(TUIData *tui, Integer status) - FUNC_ATTR_NONNULL_ALL -{ - tui->seen_error_exit = (int)status; -} - void tui_stop(TUIData *tui) FUNC_ATTR_NONNULL_ALL { @@ -660,8 +654,6 @@ static void tui_terminal_stop(TUIData *tui) FUNC_ATTR_NONNULL_ALL { tinput_stop(&tui->input); - // Position the cursor on the last screen line, below all the text - cursor_goto(tui, tui->height - 1, 0); terminfo_stop(tui); } diff --git a/src/nvim/ui_client.c b/src/nvim/ui_client.c index 577e425211..9a48f51220 100644 --- a/src/nvim/ui_client.c +++ b/src/nvim/ui_client.c @@ -348,6 +348,16 @@ cleanup: restart_args = (Array)ARRAY_DICT_INIT; } +void ui_client_event_error_exit(Array args) +{ + if (args.size < 1 + || args.items[0].type != kObjectTypeInteger) { + ELOG("Error handling ui event 'error_exit'"); + return; + } + ui_client_error_exit = (int)args.items[0].data.integer; +} + #ifdef EXITFREE void ui_client_free_all_mem(void) { diff --git a/src/nvim/ui_client.h b/src/nvim/ui_client.h index 9032135ddf..7022e43786 100644 --- a/src/nvim/ui_client.h +++ b/src/nvim/ui_client.h @@ -17,7 +17,11 @@ EXTERN sattr_T *grid_line_buf_attr INIT( = NULL); // Client-side UI channel. Zero during early startup or if not a (--remote-ui) UI client. EXTERN uint64_t ui_client_channel_id INIT( = 0); -// exit status from embedded nvim process +/// `status` argument of the last "error_exit" UI event, or -1 if none has been seen. +/// NOTE: This assumes "error_exit" never has a negative `status` argument. +EXTERN int ui_client_error_exit INIT( = -1); + +/// Server exit code. EXTERN int ui_client_exit_status INIT( = 0); /// Whether ui client has sent nvim_ui_attach yet diff --git a/test/functional/terminal/tui_spec.lua b/test/functional/terminal/tui_spec.lua index 5d10ace2b6..11f501faa9 100644 --- a/test/functional/terminal/tui_spec.lua +++ b/test/functional/terminal/tui_spec.lua @@ -33,6 +33,28 @@ local assert_log = t.assert_log local testlog = 'Xtest-tui-log' +describe('TUI', function() + it('exit status 1 and error message with server --listen error #34365', function() + clear() + local addr_in_use = api.nvim_get_vvar('servername') + local screen = tt.setup_child_nvim( + { '--listen', addr_in_use, '-u', 'NONE', '-i', 'NONE' }, + { extra_rows = 10, cols = 60 } + ) + -- When the address is very long, the error message may be only partly visible. + if #addr_in_use <= 600 then + screen:expect({ + any = vim.pesc( + ('%s: Failed to --listen: address already in use:'):format( + is_os('win') and 'nvim.exe' or 'nvim' + ) + ), + }) + end + screen:expect({ any = vim.pesc('[Process exited 1]') }) + end) +end) + describe('TUI :detach', function() it('does not stop server', function() local job_opts = { env = {} } @@ -2227,15 +2249,20 @@ describe('TUI', function() it('no assert failure on deadly signal #21896', function() exec_lua([[vim.uv.kill(vim.fn.jobpid(vim.bo.channel), 'sigterm')]]) - screen:expect { - grid = [[ + screen:expect([[ Nvim: Caught deadly signal 'SIGTERM' | | [Process exited 1]^ | |*3 {3:-- TERMINAL --} | - ]], - } + ]]) + end) + + it('exit status 1 and error message with deadly signal sent to server', function() + local _, server_pid = child_session:request('nvim_call_function', 'getpid', {}) + exec_lua([[vim.uv.kill(..., 'sigterm')]], server_pid) + screen:expect({ any = vim.pesc([[Nvim: Caught deadly signal 'SIGTERM']]) }) + screen:expect({ any = vim.pesc('[Process exited 1]') }) end) it('no stack-use-after-scope with cursor color #22432', function()