[vte] lib: Rework child exit and EOF handling



commit 7888602c3a980eee093313b2c0f949c756668070
Author: Christian Persch <chpe src gnome org>
Date:   Sun Nov 17 21:58:09 2019 +0100

    lib: Rework child exit and EOF handling
    
    When the child process exits, we used to immediately unset the PTY,
    which causes us to miss data written by the child but not yet read
    by vte.
    
    Instead, only store the child exit status, and defer emitting the
    'child-exited' signal until after all the pending data has been read
    and processed.
    
    Similarly, rework how EOF is processed. Instead of immediately
    queuing the emission of the 'eof' signal, only take note of the EOF,
    and process it after all pending data has processed. There also was
    a bug in that we took the first occurence of G_IO_HUP in
    Terminal::pty_io_read() to stop reading more data. Instead, only
    take a pure G_IO_HUP without G_IO_IN as EOF, or if reading data
    from the PTY returns the EIO error.
    
    This also fixes the bug where a(ny) partial character(s) not yet fully
    decoded by the UTF-8 and ICU decoder would not show in the output; this
    now correctly flushes the decoder, which inserts either a replacement
    character (for the UTF-8 decoder) or the character(s) in the ICU decoder
    internal state (most likely also a replacement character).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=777686

 src/chunk.hh       |  15 ++-
 src/vte.cc         | 276 ++++++++++++++++++++++++++++++-----------------------
 src/vteinternal.hh |  12 ++-
 src/widget.cc      |   7 ++
 src/widget.hh      |   1 +
 5 files changed, 188 insertions(+), 123 deletions(-)
---
diff --git a/src/chunk.hh b/src/chunk.hh
index c1d8b5a9..65bce52a 100644
--- a/src/chunk.hh
+++ b/src/chunk.hh
@@ -41,14 +41,20 @@ private:
 
         static unsigned int const k_max_free_chunks = 16;
 
+        enum class Flags : uint8_t {
+                eSEALED = 1u << 0,
+                eEOS    = 1u << 1,
+        };
+
 public:
         using unique_type = std::unique_ptr<Chunk, Recycler>;
 
         static unsigned int const k_chunk_size = 0x2000;
 
         unsigned int len{0};
+        uint8_t m_flags{0};
         uint8_t dataminusone;    /* Hack: Keep it right before data, so that data[-1] is valid and usable */
-        uint8_t data[k_chunk_size - 2 * sizeof(void*) - 1 - sizeof(unsigned int)];
+        uint8_t data[k_chunk_size - 2 * sizeof(void*) - 2 - sizeof(unsigned int)];
 
         Chunk() = default;
         Chunk(Chunk const&) = delete;
@@ -61,6 +67,7 @@ public:
         void reset() noexcept
         {
                 len = 0;
+                m_flags = 0;
         }
 
         inline constexpr size_t capacity() const noexcept { return sizeof(data); }
@@ -69,6 +76,12 @@ public:
         static unique_type get() noexcept;
         static void prune(unsigned int max_size = k_max_free_chunks) noexcept;
 
+        inline constexpr bool sealed() const noexcept { return m_flags & (uint8_t)Flags::eSEALED; }
+        inline void set_sealed() noexcept { m_flags |= (uint8_t)Flags::eSEALED; }
+
+        inline constexpr bool eos() const noexcept { return m_flags & (uint8_t)Flags::eEOS; }
+        inline void set_eos() noexcept { m_flags |= (uint8_t)Flags::eEOS; }
+
 private:
 
         /* Note that this is using the standard deleter, not Recycler */
diff --git a/src/vte.cc b/src/vte.cc
index 960b041b..5b2bbe69 100644
--- a/src/vte.cc
+++ b/src/vte.cc
@@ -789,30 +789,43 @@ Terminal::queue_cursor_moved()
        m_cursor_moved_pending = true;
 }
 
-static gboolean
-emit_eof_idle_cb(VteTerminal *terminal)
+void
+Terminal::emit_eof()
 {
-        _vte_terminal_get_impl(terminal)->emit_eof();
-
-        return G_SOURCE_REMOVE;
+        if (widget())
+                widget()->emit_eof();
 }
 
 void
-Terminal::emit_eof()
+Terminal::emit_child_exited()
 {
-       _vte_debug_print(VTE_DEBUG_SIGNALS,
-                       "Emitting `eof'.\n");
-       g_signal_emit(m_terminal, signals[SIGNAL_EOF], 0);
+        auto const status = m_child_exit_status;
+        m_child_exit_status = -1;
+
+        if (widget())
+                widget()->emit_child_exited(status);
 }
 
-/* Emit a "eof" signal. */
-// FIXMEchpe any particular reason not to handle this immediately?
+static gboolean
+emit_child_exited_idle_cb(VteTerminal *terminal)
+{
+        _vte_terminal_get_impl(terminal)->emit_child_exited();
+        // @terminal might be destroyed at this point
+
+        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_eof()
+Terminal::queue_child_exited()
 {
-        _vte_debug_print(VTE_DEBUG_SIGNALS, "Queueing `eof'.\n");
+        _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_eof_idle_cb,
+                        (GSourceFunc)emit_child_exited_idle_cb,
                         g_object_ref(m_terminal),
                         g_object_unref);
 }
@@ -3060,14 +3073,8 @@ reaper_child_exited_cb(VteReaper *reaper,
                        int status,
                        vte::terminal::Terminal* that)
 {
-        pid_t pid = pid_t{ipid};
-
-        auto terminal = that->m_terminal;
-        /* keep the vte::terminal::Terminal in a death grip */
-        g_object_ref(terminal);
-        that->child_watch_done(pid, status);
-        g_object_unref(terminal);
-        /* Note: terminal may be destroyed at this point */
+        that->child_watch_done(pid_t{ipid}, status);
+        // @that might be destroyed at this point
 }
 
 void
@@ -3077,9 +3084,6 @@ Terminal::child_watch_done(pid_t pid,
        if (pid != m_pty_pid)
                 return;
 
-        GObject *object = G_OBJECT(m_terminal);
-        g_object_freeze_notify(object);
-
         _VTE_DEBUG_IF (VTE_DEBUG_LIFECYCLE) {
                 g_printerr ("Child[%d] exited with status %d\n",
                             pid, status);
@@ -3105,14 +3109,18 @@ Terminal::child_watch_done(pid_t pid,
 
         m_pty_pid = -1;
 
-        /* Close out the PTY. */
-        unset_pty();
-
-        /* Tell observers what's happened. */
-        if (m_real_widget)
-                m_real_widget->emit_child_exited(status);
+        /* 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;
+        } else {
+                m_child_exited_after_eos_pending = false;
 
-        g_object_thaw_notify(object);
+                if (widget())
+                        widget()->emit_child_exited(status);
+        }
 }
 
 static void
@@ -3141,7 +3149,7 @@ Terminal::connect_pty_read()
 
         m_pty_input_source = g_unix_fd_add_full(VTE_CHILD_INPUT_PRIORITY,
                                                 pty()->fd(),
-                                                (GIOCondition)(G_IO_IN | G_IO_PRI | G_IO_HUP),
+                                                (GIOCondition)(G_IO_IN | G_IO_PRI | G_IO_HUP | G_IO_ERR),
                                                 (GUnixFDSourceFunc)io_read_cb,
                                                 this,
                                                 (GDestroyNotify)mark_input_source_invalid_cb);
@@ -3280,22 +3288,6 @@ Terminal::watch_child (pid_t child_pid)
         g_object_thaw_notify(object);
 }
 
-/* Handle an EOF from the client. */
-void
-Terminal::pty_channel_eof()
-{
-        GObject *object = G_OBJECT(m_terminal);
-
-        g_object_freeze_notify(object);
-
-        unset_pty();
-
-       /* Emit a signal that we read an EOF. */
-       queue_eof();
-
-        g_object_thaw_notify(object);
-}
-
 /* Reset the input method context. */
 void
 Terminal::im_reset()
@@ -3518,6 +3510,16 @@ Terminal::process_incoming_utf8()
                         }
                         }
                 }
+
+                if (chunk->eos()) {
+                        m_eos_pending = true;
+                        /* If there's an unfinished character in the queue, insert a replacement character */
+                        if (m_utf8_decoder.flush()) {
+                                insert_char(m_utf8_decoder.codepoint(), false, true);
+                        }
+
+                        break;
+                }
         }
 
 #ifdef VTE_DEBUG
@@ -3660,9 +3662,11 @@ Terminal::process_incoming_pcterm()
                 auto const* ip = chunk->data;
                 auto const* iend = chunk->data + chunk->len;
 
+                auto eos = bool{false};
                 auto flush = bool{false};
-                while (ip < iend || flush) {
 
+        start:
+                while (ip < iend || flush) {
                         switch (decoder.decode(&ip, flush)) {
                         case vte::base::ICUDecoder::Result::eSomething: {
                                 auto rv = m_parser.feed(decoder.codepoint());
@@ -3789,6 +3793,18 @@ Terminal::process_incoming_pcterm()
 
                         }
                 }
+
+                if (eos) {
+                        /* Done processing the last chunk */
+                        m_eos_pending = true;
+                        break;
+                }
+
+                if (chunk->eos()) {
+                        /* On EOS, we still need to flush the decoder before we can finish */
+                        eos = flush = true;
+                        goto start;
+                }
         }
 
 #ifdef VTE_DEBUG
@@ -3869,15 +3885,24 @@ bool
 Terminal::pty_io_read(int const fd,
                       GIOCondition const condition)
 {
-       int err = 0;
-       gboolean eof, again = TRUE;
-
        _vte_debug_print (VTE_DEBUG_WORK, ".");
 
-       /* Check for end-of-file. */
-       eof = condition & G_IO_HUP;
+        /* We need to check for EOS so that we can shut down the PTY.
+         * When we get G_IO_HUP without G_IO_IN, we can process the EOF now.
+         * However when we get G_IO_IN | G_IO_HUP, there is still data to be
+         * read in this round, and in potentially more rounds; read the data
+         * now, do *not* process the EOS (unless the read returns EIO, which
+         * does happen and appears to mean that despite G_IO_IN no data was
+         * actually available to be read, not even the cpkt header), and
+         * otherwise wait for further calls which will have G_IO_HUP (and
+         * possibly G_IO_IN again).
+         */
+        auto eos = bool{condition == G_IO_HUP};
 
-       /* Read some data in from this channel. */
+        /* There is data to read */
+       auto err = int{0};
+        auto again = bool{true};
+        vte::base::Chunk* chunk{nullptr};
        if (condition & (G_IO_IN | G_IO_PRI)) {
                guchar *bp;
                int rem, len;
@@ -3900,14 +3925,14 @@ Terminal::pty_io_read(int const fd,
                }
                bytes = m_input_bytes;
 
-                vte::base::Chunk* chunk = nullptr;
                 /* If possible, try adding more data to the chunk at the back of the queue */
                 if (!m_incoming_queue.empty())
                         chunk = m_incoming_queue.back().get();
 
                do {
-                        /* No chunk, or chunk at least ¾ full? Get a new chunk */
+                        /* No chunk, chunk sealed or at least ¾ full? Get a new chunk */
                        if (!chunk ||
+                            chunk->sealed() ||
                             chunk->len >= 3 * chunk->capacity() / 4) {
                                 m_incoming_queue.push(vte::base::Chunk::get());
 
@@ -3925,6 +3950,7 @@ Terminal::pty_io_read(int const fd,
                                  */
                                 char pkt_header;
                                 char save = bp[-1];
+                                errno = 0;
                                 int ret = read (fd, bp - 1, rem + 1);
                                 pkt_header = bp[-1];
                                 bp[-1] = save;
@@ -3933,33 +3959,36 @@ Terminal::pty_io_read(int const fd,
                                                err = errno;
                                                goto out;
                                        case 0:
-                                               eof = TRUE;
+                                               eos = true;
                                                goto out;
                                        default:
                                                 ret--;
 
-                                                if (pkt_header & TIOCPKT_IOCTL) {
-                                                        /* We'd like to always be informed when the termios 
change,
-                                                         * so we can e.g. detect when no-echo is en/disabled 
and
-                                                         * change the cursor/input method/etc., but 
unfortunately
-                                                         * the kernel only sends this flag when (old or new) 
'local flags'
-                                                         * include EXTPROC, which is not used often, and due 
to its side
-                                                         * effects, cannot be enabled by vte by default.
-                                                         *
-                                                         * FIXME: improve the kernel! see discussion in bug 
755371
-                                                         * starting at comment 12
-                                                         */
-                                                        pty_termios_changed();
-                                                }
-                                                if (pkt_header & TIOCPKT_STOP) {
-                                                        pty_scroll_lock_changed(true);
-                                                } else if (pkt_header & TIOCPKT_START) {
-                                                        pty_scroll_lock_changed(false);
+                                                if (pkt_header == TIOCPKT_DATA) {
+                                                        bp += ret;
+                                                        rem -= ret;
+                                                        len += ret;
+                                                } else {
+                                                        if (pkt_header & TIOCPKT_IOCTL) {
+                                                                /* We'd like to always be informed when the 
termios change,
+                                                                 * so we can e.g. detect when no-echo is 
en/disabled and
+                                                                 * change the cursor/input method/etc., but 
unfortunately
+                                                                 * the kernel only sends this flag when (old 
or new) 'local flags'
+                                                                 * include EXTPROC, which is not used often, 
and due to its side
+                                                                 * effects, cannot be enabled by vte by 
default.
+                                                                 *
+                                                                 * FIXME: improve the kernel! see discussion 
in bug 755371
+                                                                 * starting at comment 12
+                                                                 */
+                                                                pty_termios_changed();
+                                                        }
+                                                        if (pkt_header & TIOCPKT_STOP) {
+                                                                pty_scroll_lock_changed(true);
+                                                        }
+                                                        if (pkt_header & TIOCPKT_START) {
+                                                                pty_scroll_lock_changed(false);
+                                                        }
                                                 }
-
-                                               bp += ret;
-                                               rem -= ret;
-                                               len += ret;
                                                break;
                                }
                        } while (rem);
@@ -3986,29 +4015,38 @@ out:
                                m_pty_input_active ? "yes" : "no");
        }
 
+        if (condition & G_IO_ERR)
+                err = EIO;
+
        /* Error? */
        switch (err) {
-               case 0: /* no error */
-                       break;
-               case EIO: /* Fake an EOF. */
-                       eof = TRUE;
-                       break;
-               case EAGAIN:
-               case EBUSY: /* do nothing */
-                       break;
-               default:
-                       /* Translators: %s is replaced with error message returned by strerror(). */
-                       g_warning (_("Error reading from child: " "%s."),
-                                       g_strerror (err));
-                       break;
+        case 0: /* no error */
+                break;
+        case EIO: /* EOS */
+                eos = true;
+                break;
+        case EAGAIN:
+        case EBUSY: /* do nothing */
+                break;
+        default:
+                _vte_debug_print (VTE_DEBUG_IO, "Error reading from child: %m");
+                break;
        }
 
-       /* If we detected an eof condition, signal one. */
-       if (eof) {
-                pty_channel_eof();
+        if (eos) {
+                /* Make a note of the EOS; but do not process it since there may be data
+                 * to be processed first in the incomding queue.
+                 */
+                if (!chunk || chunk->sealed()) {
+                        m_incoming_queue.push(vte::base::Chunk::get());
+                        chunk = m_incoming_queue.back().get();
+                }
+
+                chunk->set_sealed();
+                chunk->set_eos();
 
-               again = FALSE;
-       }
+                again = false;
+        }
 
        return again;
 }
@@ -8173,7 +8211,7 @@ Terminal::~Terminal()
        int sel;
 
         terminate_child();
-        unset_pty(false /* don't notify widget */, false /* don't process remaining data */);
+        unset_pty(false /* don't notify widget */);
         remove_update_timeout(this);
 
         /* Stop processing input. */
@@ -10165,8 +10203,7 @@ Terminal::reset(bool clear_tabstops,
 }
 
 void
-Terminal::unset_pty(bool notify_widget,
-                    bool process_remaining)
+Terminal::unset_pty(bool notify_widget)
 {
         /* This may be called from inside or from widget,
          * and must notify the widget if not called from it.
@@ -10175,23 +10212,15 @@ Terminal::unset_pty(bool notify_widget,
         disconnect_pty_read();
         disconnect_pty_write();
 
-        /* Take one last shot at processing whatever data is pending,
-         * then flush the buffers in case we're about to run a new
-         * command, disconnecting the timeout. */
-        if (!m_incoming_queue.empty() && process_remaining) {
-                process_incoming();
-                while (!m_incoming_queue.empty())
-                        m_incoming_queue.pop();
+        /* Clear incoming and outgoing queues */
+        m_input_bytes = 0;
+        m_incoming_queue = {};
+        _vte_byte_array_clear(m_outgoing);
 
-                m_input_bytes = 0;
-        }
-        stop_processing(this);
+        stop_processing(this); // FIXMEchpe only if m_incoming_queue.empty() !!!
 
         reset_decoder();
 
-        /* Clear the outgoing buffer as well. */
-        _vte_byte_array_clear(m_outgoing);
-
         m_pty.reset();
 
         if (notify_widget && widget())
@@ -10199,14 +10228,13 @@ Terminal::unset_pty(bool notify_widget,
 }
 
 bool
-Terminal::set_pty(vte::base::Pty *new_pty,
-                  bool process_remaining)
+Terminal::set_pty(vte::base::Pty *new_pty)
 {
         if (pty().get() == new_pty)
                 return false;
 
         if (pty()) {
-                unset_pty(false /* don't notify widget */, process_remaining);
+                unset_pty(false /* don't notify widget */);
         }
 
         m_pty = vte::base::make_ref(new_pty);
@@ -10483,6 +10511,20 @@ Terminal::emit_pending_signals()
         }
 
         g_object_thaw_notify(object);
+
+        auto const eos = m_eos_pending;
+        if (m_eos_pending) {
+                emit_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 38e98e2a..ba40d2bc 100644
--- a/src/vteinternal.hh
+++ b/src/vteinternal.hh
@@ -359,15 +359,16 @@ public:
         vte::base::RefPtr<vte::base::Pty> m_pty{};
         inline constexpr auto& pty() const noexcept { return m_pty; }
 
-        void unset_pty(bool notify_widget = true,
-                       bool process_remaining = true);
-        bool set_pty(vte::base::Pty* pty,
-                     bool process_remaining = true);
+        void unset_pty(bool notify_widget = true);
+        bool set_pty(vte::base::Pty* pty);
 
         guint m_pty_input_source{0};
         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};
         VteReaper *m_reaper;
 
        /* Queue of chunks of data read from the PTY.
@@ -973,6 +974,7 @@ public:
         bool terminate_child () noexcept;
         void child_watch_done(pid_t pid,
                               int status);
+        void emit_child_exited();
 
         void im_commit(char const* text);
         void im_preedit_set_active(bool active) noexcept;
@@ -1108,7 +1110,7 @@ public:
 
         void queue_cursor_moved();
         void queue_contents_changed();
-        void queue_eof();
+        void queue_child_exited();
 
         void emit_text_deleted();
         void emit_text_inserted();
diff --git a/src/widget.cc b/src/widget.cc
index eef88311..aee1c56c 100644
--- a/src/widget.cc
+++ b/src/widget.cc
@@ -181,6 +181,13 @@ Widget::emit_child_exited(int status) noexcept
         g_signal_emit(object(), signals[SIGNAL_CHILD_EXITED], 0, status);
 }
 
+void
+Widget::emit_eof() noexcept
+{
+        _vte_debug_print(VTE_DEBUG_SIGNALS, "Emitting `eof'.\n");
+        g_signal_emit(object(), signals[SIGNAL_EOF], 0);
+}
+
 bool
 Widget::im_filter_keypress(GdkEventKey* event) noexcept
 {
diff --git a/src/widget.hh b/src/widget.hh
index 5e3b3ac4..251a1fbe 100644
--- a/src/widget.hh
+++ b/src/widget.hh
@@ -107,6 +107,7 @@ public:
         char const* encoding() const noexcept { return m_terminal->encoding(); }
 
         void emit_child_exited(int status) noexcept;
+        void emit_eof() noexcept;
 
         bool set_pty(VtePty* pty) noexcept;
         inline auto pty() const noexcept { return m_pty.get(); }


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]