From 36b37559cf63760c5b3e717384963baa866273f3 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Sun, 23 Jan 2022 23:36:44 -0600 Subject: [PATCH] Fix exit regression Currently vte delays the "child-exited" signal to until after eos has been received. This is wrong and caused a regression. vte shouldn't tell client terminals when to exit, if the children has exited, vte should send the "child-exited" signal. Period. If the client decides to wait for "eof", that's up to the client terminal. On Unix if the child spawns grandchildren, their output will be attached to the controlling terminal (unless a tool like nohup is used), therefore the eos won't happen until all the grandchildren have closed their output. For example this command will keep producing output after the shell has exited: sh -c 'for i in $(seq 1 10); do echo $i; sleep 1; done' & exit vte assumes an eos will always occur, which is why if a shell spawns any grandchildren, exiting the shell doesn't close the terminal: vte would wait for all the grandchildren to exit, which from the point of view of the user it seems the terminal is hanging forever. To workaround this issue, a 5-second timer was introduced in 058adf5f (pty: Fix indefinite wait for EOS after child-exited, 2019-12-01), which was later reduced to 2 seconds, but this is just a hack. vte should not be waiting for an eos before sending "child-exited". In the past vte used to send the "child-exited" immediately, which is the correct desired behavior, but that changed in commit 7888602c (lib: Rework child exit and EOF handling, 2019-11-17). The only correct change in that commit is removing unset_pty() from child_watch_done() which does allow the code to process remaining data in the buffers, even from grandchildren that take several seconds to finish. Delaying "child-exited" was not needed to fix that problem and introduced the regression that can be easily triggered by simply doing: sleep infinity & exit Decoupling "child-exited" from "eof" gets rid of a lot of unnecessary complexity, and also eliminates the need for the hacky timer. With this change even the output from grandchildren processes that last several seconds is processed correctly: xfce4-terminal --disable-server --hold \ --command="bash -i -c \"sh -c 'for i in \$(seq 1 10); do echo \$i; sleep 1; done' & exit\"" And of course, the vastly most common case of terminals running shells that want to exit immediately works correctly. Fixes #204. Signed-off-by: Felipe Contreras --- src/vte.cc | 84 ++-------------------------------------------- src/vteinternal.hh | 8 ----- 2 files changed, 2 insertions(+), 90 deletions(-) diff --git a/src/vte.cc b/src/vte.cc index 7cc26c48..d37dacc9 100644 --- a/src/vte.cc +++ b/src/vte.cc @@ -953,62 +953,6 @@ Terminal::queue_eof() g_object_unref); } -void -Terminal::emit_child_exited() -{ - auto const status = m_child_exit_status; - m_child_exit_status = -1; - - if (widget()) - widget()->emit_child_exited(status); -} - -static gboolean -emit_child_exited_idle_cb(VteTerminal *terminal) -try -{ - _vte_terminal_get_impl(terminal)->emit_child_exited(); - - return G_SOURCE_REMOVE; -} -catch (...) -{ - vte::log_exception(); - return G_SOURCE_REMOVE; -} - -/* Emit a "child-exited" signal on idle, so that if the handler destroys - * the terminal, we're not deep within terminal code callstack - */ -void -Terminal::queue_child_exited() -{ - _vte_debug_print(VTE_DEBUG_SIGNALS, "Queueing `child-exited'.\n"); - m_child_exited_after_eos_pending = false; - - g_idle_add_full(G_PRIORITY_HIGH, - (GSourceFunc)emit_child_exited_idle_cb, - g_object_ref(m_terminal), - g_object_unref); -} - -bool -Terminal::child_exited_eos_wait_callback() -{ - /* If we get this callback, there has been some time elapsed - * after child-exited, but no EOS yet. This happens for example - * when the primary child started other processes in the background, - * which inherited the PTY, and thus keep it open, see - * https://gitlab.gnome.org/GNOME/vte/issues/204 - * - * Force an EOS. - */ - if (pty()) - pty_io_read(pty()->fd(), G_IO_HUP); - - return false; // don't run again -} - /* Emit an "increase-font-size" signal. */ void Terminal::emit_increase_font_size() @@ -3273,20 +3217,8 @@ Terminal::child_watch_done(pid_t pid, m_pty_pid = -1; - /* If we still have a PTY, or data to process, defer emitting the signals - * until we have EOF on the PTY, so that we can process all pending data. - */ - if (pty() || !m_incoming_queue.empty()) { - m_child_exit_status = status; - m_child_exited_after_eos_pending = true; - - m_child_exited_eos_wait_timer.schedule_seconds(2); // FIXME: better value? - } else { - m_child_exited_after_eos_pending = false; - - if (widget()) - widget()->emit_child_exited(status); - } + if (widget()) + widget()->emit_child_exited(status); } static void @@ -4095,9 +4027,6 @@ out: chunk->set_sealed(); chunk->set_eos(); - /* Cancel wait timer */ - m_child_exited_eos_wait_timer.abort(); - /* Need to process the EOS */ if (!is_processing()) { add_process_timeout(this); @@ -9976,8 +9905,6 @@ Terminal::unset_pty(bool notify_widget) disconnect_pty_read(); disconnect_pty_write(); - m_child_exited_eos_wait_timer.abort(); - /* Clear incoming and outgoing queues */ m_input_bytes = 0; m_incoming_queue = {}; @@ -10273,19 +10200,12 @@ Terminal::emit_pending_signals() m_bell_pending = false; } - auto const eos = m_eos_pending; if (m_eos_pending) { queue_eof(); m_eos_pending = false; unset_pty(); } - - if (m_child_exited_after_eos_pending && eos) { - /* The signal handler could destroy the terminal, so send the signal on idle */ - queue_child_exited(); - m_child_exited_after_eos_pending = false; - } } void diff --git a/src/vteinternal.hh b/src/vteinternal.hh index 0d454649..1636d511 100644 --- a/src/vteinternal.hh +++ b/src/vteinternal.hh @@ -291,13 +291,7 @@ public: guint m_pty_output_source{0}; bool m_pty_input_active{false}; pid_t m_pty_pid{-1}; /* pid of child process */ - int m_child_exit_status{-1}; /* pid's exit status, or -1 */ bool m_eos_pending{false}; - bool m_child_exited_after_eos_pending{false}; - bool child_exited_eos_wait_callback(); - vte::glib::Timer m_child_exited_eos_wait_timer{std::bind(&Terminal::child_exited_eos_wait_callback, - this), - "child-exited-eos-wait-timer"}; VteReaper *m_reaper; /* Queue of chunks of data read from the PTY. @@ -988,7 +982,6 @@ public: bool terminate_child () noexcept; void child_watch_done(pid_t pid, int status); - void emit_child_exited(); void im_commit(std::string_view const& str); void im_preedit_set_active(bool active) noexcept; @@ -1128,7 +1121,6 @@ public: void queue_cursor_moved(); void queue_contents_changed(); - void queue_child_exited(); void queue_eof(); #ifdef WITH_A11Y -- 2.39.2