Merge branch 'fallible_append'

Closes #9266.

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<bool(const history_item_t &item)> 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;
 }
 

src/io.cpp

@@ -206,9 +206,10 @@ void io_chain_t::push_back(io_data_ref_t element) {
     std::vector<io_data_ref_t>::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<const io_data_t> 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,16 +334,26 @@ 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.
-        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;
     }
+    return !errored_;
 }
 
 int fd_output_stream_t::flush_and_check_error() {
@@ -347,20 +361,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() {

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<element_t> &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<io_data_ref_t> {
 
     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<io_data_ref_t> {
 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;