From 8e97fcb22c9555945844645c291a4ab45eb90307 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sat, 8 Oct 2022 12:24:27 -0500 Subject: [PATCH 1/3] Make `output_stream_t::append()` fallible Allow errors encountered by certain implementations of `output_stream_t` when writing to the output sink to be bubbled back to the caller. --- src/io.cpp | 36 +++++++++++++++++++------------ src/io.h | 63 ++++++++++++++++++++++++++++++++---------------------- 2 files changed, 60 insertions(+), 39 deletions(-) diff --git a/src/io.cpp b/src/io.cpp index 0f0449848..fa1dbd182 100644 --- a/src/io.cpp +++ b/src/io.cpp @@ -206,9 +206,10 @@ void io_chain_t::push_back(io_data_ref_t element) { std::vector::push_back(std::move(element)); } -void io_chain_t::append(const io_chain_t &chain) { +bool io_chain_t::append(const io_chain_t &chain) { assert(&chain != this && "Cannot append self to self"); this->insert(this->end(), chain.begin(), chain.end()); + return true; } bool io_chain_t::append_from_specs(const redirection_spec_list_t &specs, const wcstring &pwd) { @@ -308,21 +309,24 @@ shared_ptr io_chain_t::io_for_fd(int fd) const { return nullptr; } -void output_stream_t::append_narrow_buffer(const separated_buffer_t &buffer) { +bool output_stream_t::append_narrow_buffer(const separated_buffer_t &buffer) { for (const auto &rhs_elem : buffer.elements()) { - append_with_separation(str2wcstring(rhs_elem.contents), rhs_elem.separation, false); + if (!append_with_separation(str2wcstring(rhs_elem.contents), rhs_elem.separation, false)) { + return false; + } } + return true; } -void output_stream_t::append_with_separation(const wchar_t *s, size_t len, separation_type_t type, +bool output_stream_t::append_with_separation(const wchar_t *s, size_t len, separation_type_t type, bool want_newline) { if (type == separation_type_t::explicitly && want_newline) { // Try calling "append" less - it might write() to an fd wcstring buf{s, len}; buf.push_back(L'\n'); - append(buf); + return append(buf); } else { - append(s, len); + return append(s, len); } } @@ -330,8 +334,8 @@ const wcstring &output_stream_t::contents() const { return g_empty_string; } int output_stream_t::flush_and_check_error() { return STATUS_CMD_OK; } -void fd_output_stream_t::append(const wchar_t *s, size_t amt) { - if (errored_) return; +bool fd_output_stream_t::append(const wchar_t *s, size_t amt) { + if (errored_) return false; int res = wwrite_to_fd(s, amt, this->fd_); if (res < 0) { // TODO: this error is too aggressive, e.g. if we got SIGINT we should not complain. @@ -340,6 +344,7 @@ void fd_output_stream_t::append(const wchar_t *s, size_t amt) { } errored_ = true; } + return !errored_; } int fd_output_stream_t::flush_and_check_error() { @@ -347,20 +352,23 @@ int fd_output_stream_t::flush_and_check_error() { return errored_ ? STATUS_CMD_ERROR : STATUS_CMD_OK; } -void null_output_stream_t::append(const wchar_t *, size_t) {} +bool null_output_stream_t::append(const wchar_t *, size_t) { return true; } -void string_output_stream_t::append(const wchar_t *s, size_t amt) { contents_.append(s, amt); } +bool string_output_stream_t::append(const wchar_t *s, size_t amt) { + contents_.append(s, amt); + return true; +} const wcstring &string_output_stream_t::contents() const { return contents_; } -void buffered_output_stream_t::append(const wchar_t *s, size_t amt) { - buffer_->append(wcs2string(s, amt)); +bool buffered_output_stream_t::append(const wchar_t *s, size_t amt) { + return buffer_->append(wcs2string(s, amt)); } -void buffered_output_stream_t::append_with_separation(const wchar_t *s, size_t len, +bool buffered_output_stream_t::append_with_separation(const wchar_t *s, size_t len, separation_type_t type, bool want_newline) { UNUSED(want_newline); - buffer_->append(wcs2string(s, len), type); + return buffer_->append(wcs2string(s, len), type); } int buffered_output_stream_t::flush_and_check_error() { diff --git a/src/io.h b/src/io.h index 436eab965..bd23eddf4 100644 --- a/src/io.h +++ b/src/io.h @@ -16,6 +16,8 @@ #include "fds.h" #include "global_safety.h" #include "redirection.h" +#include "signal.h" +#include "topic_monitor.h" using std::shared_ptr; @@ -82,25 +84,27 @@ class separated_buffer_t : noncopyable_t { const std::vector &elements() const { return elements_; } /// Append a string \p str of a given length \p len, with separation type \p sep. - void append(const char *str, size_t len, separation_type_t sep = separation_type_t::inferred) { - if (!try_add_size(len)) return; + bool append(const char *str, size_t len, separation_type_t sep = separation_type_t::inferred) { + if (!try_add_size(len)) return false; // Try merging with the last element. if (sep == separation_type_t::inferred && last_inferred()) { elements_.back().contents.append(str, len); } else { elements_.emplace_back(std::string(str, len), sep); } + return true; } /// Append a string \p str with separation type \p sep. - void append(std::string &&str, separation_type_t sep = separation_type_t::inferred) { - if (!try_add_size(str.size())) return; + bool append(std::string &&str, separation_type_t sep = separation_type_t::inferred) { + if (!try_add_size(str.size())) return false; // Try merging with the last element. if (sep == separation_type_t::inferred && last_inferred()) { elements_.back().contents.append(str); } else { elements_.emplace_back(std::move(str), sep); } + return true; } /// Remove all elements and unset the discard flag. @@ -280,8 +284,8 @@ class io_buffer_t { ~io_buffer_t(); /// Append a string to the buffer. - void append(std::string &&str, separation_type_t type = separation_type_t::inferred) { - buffer_.acquire()->append(std::move(str), type); + bool append(std::string &&str, separation_type_t type = separation_type_t::inferred) { + return buffer_.acquire()->append(std::move(str), type); } /// \return true if output was discarded due to exceeding the read limit. @@ -328,7 +332,7 @@ class io_chain_t : public std::vector { void remove(const io_data_ref_t &element); void push_back(io_data_ref_t element); - void append(const io_chain_t &chain); + bool append(const io_chain_t &chain); /// \return the last io redirection in the chain for the specified file descriptor, or nullptr /// if none. @@ -347,7 +351,7 @@ class io_chain_t : public std::vector { class output_stream_t : noncopyable_t, nonmovable_t { public: /// Required override point. The output stream receives a string \p s with \p amt chars. - virtual void append(const wchar_t *s, size_t amt) = 0; + virtual bool append(const wchar_t *s, size_t amt) = 0; /// \return any internally buffered contents. /// This is only implemented for a string_output_stream; others flush data to their underlying @@ -361,35 +365,39 @@ class output_stream_t : noncopyable_t, nonmovable_t { /// An optional override point. This is for explicit separation. /// \param want_newline this is true if the output item should be ended with a newline. This /// is only relevant if we are printing the output to a stream, - virtual void append_with_separation(const wchar_t *s, size_t len, separation_type_t type, + virtual bool append_with_separation(const wchar_t *s, size_t len, separation_type_t type, bool want_newline = true); /// The following are all convenience overrides. - void append_with_separation(const wcstring &s, separation_type_t type, + bool append_with_separation(const wcstring &s, separation_type_t type, bool want_newline = true) { - append_with_separation(s.data(), s.size(), type, want_newline); + return append_with_separation(s.data(), s.size(), type, want_newline); } /// Append a string. - void append(const wcstring &s) { append(s.data(), s.size()); } - void append(const wchar_t *s) { append(s, std::wcslen(s)); } + bool append(const wcstring &s) { return append(s.data(), s.size()); } + bool append(const wchar_t *s) { return append(s, std::wcslen(s)); } /// Append a char. - void append(wchar_t s) { append(&s, 1); } - void push_back(wchar_t c) { append(c); } + bool append(wchar_t s) { return append(&s, 1); } + bool push_back(wchar_t c) { return append(c); } // Append data from a narrow buffer, widening it. - void append_narrow_buffer(const separated_buffer_t &buffer); + bool append_narrow_buffer(const separated_buffer_t &buffer); /// Append a format string. - void append_format(const wchar_t *format, ...) { + bool append_format(const wchar_t *format, ...) { va_list va; va_start(va, format); - append_formatv(format, va); + bool r = append_formatv(format, va); va_end(va); + + return r; } - void append_formatv(const wchar_t *format, va_list va) { append(vformat_string(format, va)); } + bool append_formatv(const wchar_t *format, va_list va) { + return append(vformat_string(format, va)); + } output_stream_t() = default; virtual ~output_stream_t() = default; @@ -397,7 +405,7 @@ class output_stream_t : noncopyable_t, nonmovable_t { /// A null output stream which ignores all writes. class null_output_stream_t final : public output_stream_t { - virtual void append(const wchar_t *s, size_t amt) override; + virtual bool append(const wchar_t *s, size_t amt) override; }; /// An output stream for builtins which outputs to an fd. @@ -405,16 +413,21 @@ class null_output_stream_t final : public output_stream_t { class fd_output_stream_t final : public output_stream_t { public: /// Construct from a file descriptor, which must be nonegative. - explicit fd_output_stream_t(int fd) : fd_(fd) { assert(fd_ >= 0 && "Invalid fd"); } + explicit fd_output_stream_t(int fd) : fd_(fd), sigcheck_(topic_t::sighupint) { + assert(fd_ >= 0 && "Invalid fd"); + } int flush_and_check_error() override; - void append(const wchar_t *s, size_t amt) override; + bool append(const wchar_t *s, size_t amt) override; private: /// The file descriptor to write to. const int fd_; + /// Used to check if a SIGINT has been received when EINTR is encountered + sigchecker_t sigcheck_; + /// Whether we have received an error. bool errored_{false}; }; @@ -423,7 +436,7 @@ class fd_output_stream_t final : public output_stream_t { class string_output_stream_t final : public output_stream_t { public: string_output_stream_t() = default; - void append(const wchar_t *s, size_t amt) override; + bool append(const wchar_t *s, size_t amt) override; /// \return the wcstring containing the output. const wcstring &contents() const override; @@ -440,8 +453,8 @@ class buffered_output_stream_t final : public output_stream_t { assert(buffer_ && "Buffer must not be null"); } - void append(const wchar_t *s, size_t amt) override; - void append_with_separation(const wchar_t *s, size_t len, separation_type_t type, + bool append(const wchar_t *s, size_t amt) override; + bool append_with_separation(const wchar_t *s, size_t len, separation_type_t type, bool want_newline) override; int flush_and_check_error() override; From 83636fa599d17bb87f868eaaab14413b529cf3e5 Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sat, 8 Oct 2022 12:26:29 -0500 Subject: [PATCH 2/3] Silently handle fd_output_stream_t append errors in case of SIGINT If EINTR caused by SIGINT is encountered while writing to the `fd_output_stream_t` output fd, mark the output stream as errored and return false to the caller but do not visibly complain. Addressing the outstanding TODO notwithstanding, this is needed to avoid littering the tty with spurious errors when the user hits Ctrl-C to abort a long-running builtin's output (w/ the primary example being `history`). --- src/io.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/io.cpp b/src/io.cpp index fa1dbd182..5ba1b3c3b 100644 --- a/src/io.cpp +++ b/src/io.cpp @@ -338,8 +338,17 @@ bool fd_output_stream_t::append(const wchar_t *s, size_t amt) { if (errored_) return false; int res = wwrite_to_fd(s, amt, this->fd_); if (res < 0) { - // TODO: this error is too aggressive, e.g. if we got SIGINT we should not complain. - if (errno != EPIPE) { + // Some of our builtins emit multiple screens worth of data sent to a pager (the primary + // example being the `history` builtin) and receiving SIGINT should be considered normal and + // non-exceptional (user request to abort via Ctrl-C), meaning we shouldn't print an error. + if (errno == EINTR && sigcheck_.check()) { + // We have two options here: we can either return false without setting errored_ to + // true (*this* write will be silently aborted but the onus is on the caller to check + // the return value and skip future calls to `append()`) or we can flag the entire + // output stream as errored, causing us to both return false and skip any future writes. + // We're currently going with the latter, especially seeing as no callers currently + // check the result of `append()` (since it was always a void function before). + } else if (errno != EPIPE) { wperror(L"write"); } errored_ = true; From 920ded26b9d0d0adca52826d4dc8cd02a7f2b83c Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Sat, 8 Oct 2022 12:30:17 -0500 Subject: [PATCH 3/3] history: Handle Ctrl-C/SIGINT or other errors on output append When there are multiple screens worth of output and `history` is writing to the pager, pressing Ctrl-C at the end of a screen doesn't exit the pager (`q` is needed for that) but previously caused fish to emit an error ("write: Interrupted system call) until we starting silently handling SIGINT in `fd_output_stream_t::append()`. This patch makes `history` detect when the `append()` call returns with an error and causes it to end early rather than repeatedly trying (and failing) to write to the output stream. --- src/history.cpp | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/history.cpp b/src/history.cpp index 41735bfcd..9ab51723c 100644 --- a/src/history.cpp +++ b/src/history.cpp @@ -1482,8 +1482,10 @@ bool history_t::search(history_search_type_t search_type, const wcstring_list_t wcstring_list_t collected; wcstring formatted_record; size_t remaining = max_items; + bool output_error = false; - // The function we use to act on each item. + // The function we use to act on each item. The return value indicates whether the search should + // continue (true) or stop (on false). std::function func = [&](const history_item_t &item) -> bool { if (remaining == 0) return false; remaining -= 1; @@ -1493,7 +1495,11 @@ bool history_t::search(history_search_type_t search_type, const wcstring_list_t collected.push_back(std::move(formatted_record)); } else { // We can output this immediately. - streams.out.append(formatted_record); + if (!streams.out.append(formatted_record)) { + // This can happen if the user hit Ctrl-C to abort (maybe after the first page?). + output_error = true; + return false; + } } return true; }; @@ -1514,9 +1520,16 @@ bool history_t::search(history_search_type_t search_type, const wcstring_list_t } // Output any items we collected (which only happens in reverse). - for (auto iter = collected.rbegin(); iter != collected.rend(); ++iter) { - streams.out.append(*iter); + for (auto iter = collected.rbegin(); !output_error && iter != collected.rend(); ++iter) { + if (!streams.out.append(*iter)) { + // Don't force an error if output was aborted (typically via Ctrl-C/SIGINT); just don't + // try writing any more. + output_error = true; + } } + + // We are intentionally not returning false in case of an output error, as the user aborting the + // output early (the most common case) isn't a reason to exit w/ a non-zero status code. return true; }