Introduce termsize_container_t

fish's handling of terminal sizes is currently rather twisted. The
essential problem is that the terminal size may change at any point from a
SIGWINCH, and common_get_{width,height} may modify it and post variable
change events from arbitrary locations.

Tighten up the semantics. Assign responsibility for managing the tty size
to a new class, `termsize_container_t`. Rationalize locking and reentrancy.

Explicitly nail down the relationship between $COLUMNS/$LINES and the tty
size. The new semantics are: whatever changed most recently takes
precendence.

CMakeLists.txt

@@ -121,7 +121,7 @@ set(FISH_SRCS
     src/wcstringutil.cpp src/wgetopt.cpp src/wildcard.cpp src/wutil.cpp
     src/future_feature_flags.cpp src/redirection.cpp src/topic_monitor.cpp
     src/flog.cpp src/trace.cpp src/timer.cpp src/null_terminated_array.cpp
-    src/operation_context.cpp src/fd_monitor.cpp
+    src/operation_context.cpp src/fd_monitor.cpp src/termsize.cpp
 )
 
 # Header files are just globbed.

src/env.cpp

@@ -32,6 +32,7 @@
 #include "path.h"
 #include "proc.h"
 #include "reader.h"
+#include "termsize.h"
 #include "wutil.h"  // IWYU pragma: keep
 
 /// Some configuration path environment variables.
@@ -49,10 +50,6 @@ extern char **environ;
 static constexpr wchar_t PATH_ARRAY_SEP = L':';
 static constexpr wchar_t NONPATH_ARRAY_SEP = L' ';
 
-// Default terminal sizes.
-static constexpr size_t DFLT_TERM_COL = 80;
-static constexpr size_t DFLT_TERM_ROW = 24;
-
 bool curses_initialized = false;
 
 /// Does the terminal have the "eat_newline_glitch".
@@ -363,10 +360,11 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
     }
 
     // Initialize termsize variables.
+    auto termsize = termsize_container_t::shared().initialize(vars);
     if (vars.get(L"COLUMNS").missing_or_empty())
-        vars.set_one(L"COLUMNS", ENV_GLOBAL, to_string(DFLT_TERM_COL));
+        vars.set_one(L"COLUMNS", ENV_GLOBAL, to_string(termsize.width));
     if (vars.get(L"LINES").missing_or_empty())
-        vars.set_one(L"LINES", ENV_GLOBAL, to_string(DFLT_TERM_ROW));
+        vars.set_one(L"LINES", ENV_GLOBAL, to_string(termsize.height));
 
     // Set fish_bind_mode to "default".
     vars.set_one(FISH_BIND_MODE_VAR, ENV_GLOBAL, DEFAULT_BIND_MODE);

src/fish_tests.cpp

@@ -73,6 +73,7 @@
 #include "redirection.h"
 #include "screen.h"
 #include "signal.h"
+#include "termsize.h"
 #include "timer.h"
 #include "tnode.h"
 #include "tokenizer.h"
@@ -5743,6 +5744,72 @@ Executed in  500.00 micros    fish         external
     free(saved_locale);
 }
 
+struct termsize_tester_t {
+    static void test();
+};
+
+void termsize_tester_t::test() {
+    say(L"Testing termsize");
+
+    parser_t &parser = parser_t::principal_parser();
+    env_stack_t &vars = parser.vars();
+
+    // Use a static variable so we can pretend we're the kernel exposing a terminal size.
+    static maybe_t<termsize_t> stubby_termsize{};
+    termsize_container_t ts([] { return stubby_termsize; });
+
+    // Initially default value.
+    do_test(ts.last() == termsize_t::defaults());
+
+    // Haha we change the value, it doesn't even know.
+    stubby_termsize = termsize_t{42, 84};
+    do_test(ts.last() == termsize_t::defaults());
+
+    // Ok let's tell it. But it still doesn't update right away.
+    ts.handle_winch();
+    do_test(ts.last() == termsize_t::defaults());
+
+    // Ok now we tell it to update.
+    ts.updating(parser);
+    do_test(ts.last() == *stubby_termsize);
+    do_test(vars.get(L"COLUMNS")->as_string() == L"42");
+    do_test(vars.get(L"LINES")->as_string() == L"84");
+
+    // Wow someone set COLUMNS and LINES to a weird value.
+    // Now the tty's termsize doesn't matter.
+    vars.set(L"COLUMNS", ENV_GLOBAL, {L"75"});
+    vars.set(L"LINES", ENV_GLOBAL, {L"150"});
+    ts.handle_columns_lines_var_change(vars);
+    do_test(ts.last() == termsize_t(75, 150));
+    do_test(vars.get(L"COLUMNS")->as_string() == L"75");
+    do_test(vars.get(L"LINES")->as_string() == L"150");
+
+    vars.set(L"COLUMNS", ENV_GLOBAL, {L"33"});
+    ts.handle_columns_lines_var_change(vars);
+    do_test(ts.last() == termsize_t(33, 150));
+
+    // Oh it got SIGWINCH, now the tty matters again.
+    ts.handle_winch();
+    do_test(ts.last() == termsize_t(33, 150));
+    do_test(ts.updating(parser) == *stubby_termsize);
+    do_test(vars.get(L"COLUMNS")->as_string() == L"42");
+    do_test(vars.get(L"LINES")->as_string() == L"84");
+
+    // Test initialize().
+    vars.set(L"COLUMNS", ENV_GLOBAL, {L"83"});
+    vars.set(L"LINES", ENV_GLOBAL, {L"38"});
+    ts.initialize(vars);
+    do_test(ts.last() == termsize_t(83, 38));
+
+    // initialize() even beats the tty reader until a sigwinch.
+    termsize_container_t ts2([] { return stubby_termsize; });
+    ts.initialize(vars);
+    ts2.updating(parser);
+    do_test(ts.last() == termsize_t(83, 38));
+    ts2.handle_winch();
+    do_test(ts2.updating(parser) == *stubby_termsize);
+}
+
 /// Main test.
 int main(int argc, char **argv) {
     UNUSED(argc);
@@ -5876,6 +5943,8 @@ int main(int argc, char **argv) {
     if (should_test_function("timer_format")) test_timer_format();
     // history_tests_t::test_history_speed();
 
+    if (should_test_function("termsize")) termsize_tester_t::test();
+
     say(L"Encountered %d errors in low-level tests", err_count);
     if (s_test_run_count == 0) say(L"*** No Tests Were Actually Run! ***");
 

src/signal.cpp

@@ -16,6 +16,7 @@
 #include "proc.h"
 #include "reader.h"
 #include "signal.h"
+#include "termsize.h"
 #include "topic_monitor.h"
 #include "wutil.h"  // IWYU pragma: keep
 
@@ -219,7 +220,8 @@ static void fish_signal_handler(int sig, siginfo_t *info, void *context) {
     switch (sig) {
 #ifdef SIGWINCH
         case SIGWINCH:
-            /// Respond to a winch signal by checking the terminal size.
+            /// Respond to a winch signal by invalidating the terminal size.
+            termsize_container_t::handle_winch();
             common_handle_winch(sig);
             break;
 #endif

src/termsize.cpp

@@ -0,0 +1,127 @@
+// Support for exposing the terminal size.
+
+#include "termsize.h"
+
+#include "maybe.h"
+#include "parser.h"
+#include "wutil.h"
+
+// A counter which is incremented every SIGWINCH.
+// This is only updated from termsize_handle_winch().
+static volatile uint32_t s_sigwinch_gen_count{0};
+
+/// \return a termsize from ioctl, or none on error or if not supported.
+static maybe_t<termsize_t> read_termsize_from_tty() {
+    maybe_t<termsize_t> result{};
+#ifdef HAVE_WINSIZE
+    struct winsize winsize = {0, 0, 0, 0};
+    if (ioctl(STDOUT_FILENO, TIOCGWINSZ, &winsize) >= 0) {
+        result = termsize_t{winsize.ws_col, winsize.ws_row};
+    }
+#endif
+    return result;
+}
+
+// static
+termsize_container_t &termsize_container_t::shared() {
+    // Heap-allocated to avoid runtime dtor registration.
+    static termsize_container_t *res = new termsize_container_t(read_termsize_from_tty);
+    return *res;
+}
+
+termsize_t termsize_container_t::data_t::current() const {
+    // This encapsulates our ordering logic. If we have a termsize from a tty, use it; otherwise use
+    // what we have seen from the environment.
+    if (this->last_from_tty) return *this->last_from_tty;
+    if (this->last_from_env) return *this->last_from_env;
+    return termsize_t::defaults();
+}
+
+void termsize_container_t::data_t::mark_override_from_env(termsize_t ts) {
+    // Here we pretend to have an up-to-date tty value so that we will prefer the environment value.
+    this->last_from_env = ts;
+    this->last_from_tty.reset();
+    this->last_winch_gen_count = s_sigwinch_gen_count;
+}
+
+termsize_t termsize_container_t::last() const { return this->data_.acquire()->current(); }
+
+termsize_t termsize_container_t::updating(parser_t &parser) {
+    termsize_t new_size = termsize_t::defaults();
+    termsize_t prev_size = termsize_t::defaults();
+
+    // Take the lock in a local region.
+    // Capture the size before and the new size.
+    {
+        auto data = data_.acquire();
+        prev_size = data->current();
+
+        // Critical read of signal-owned variable.
+        // This must happen before the TIOCGWINSZ ioctl.
+        const uint32_t sigwinch_gen_count = s_sigwinch_gen_count;
+        if (data->last_winch_gen_count != sigwinch_gen_count) {
+            // We have received a sigwinch (or we have not yet computed the value).
+            // Apply any updates.
+            data->last_winch_gen_count = sigwinch_gen_count;
+            data->last_from_tty = this->tty_size_reader_();
+        }
+        new_size = data->current();
+    }
+
+    // Announce any updates.
+    if (new_size != prev_size) set_columns_lines_vars(new_size, parser);
+    return new_size;
+}
+
+void termsize_container_t::set_columns_lines_vars(termsize_t val, parser_t &parser) {
+    const bool saved = setting_env_vars_;
+    setting_env_vars_ = true;
+    parser.set_var_and_fire(L"COLUMNS", ENV_GLOBAL, to_string(val.width));
+    parser.set_var_and_fire(L"LINES", ENV_GLOBAL, to_string(val.height));
+    setting_env_vars_ = saved;
+}
+
+/// Convert an environment variable to an int, or return a default value.
+/// The int must be >0 and <USHRT_MAX (from struct winsize).
+static int var_to_int_or(const maybe_t<env_var_t> &var, int def) {
+    if (var.has_value() && !var->empty()) {
+        errno = 0;
+        int proposed = fish_wcstoi(var->as_string().c_str());
+        if (errno == 0 && proposed > 0 && proposed <= USHRT_MAX) {
+            return proposed;
+        }
+    }
+    return def;
+}
+
+termsize_t termsize_container_t::initialize(const environment_t &vars) {
+    termsize_t new_termsize{
+        var_to_int_or(vars.get(L"COLUMNS", ENV_GLOBAL), -1),
+        var_to_int_or(vars.get(L"LINES", ENV_GLOBAL), -1),
+    };
+    auto data = data_.acquire();
+    if (new_termsize.width > 0 && new_termsize.height > 0) {
+        data->mark_override_from_env(new_termsize);
+    } else {
+        data->last_winch_gen_count = s_sigwinch_gen_count;
+        data->last_from_tty = this->tty_size_reader_();
+    }
+    return data->current();
+}
+
+void termsize_container_t::handle_columns_lines_var_change(const environment_t &vars) {
+    // Do nothing if we are the ones setting it.
+    if (setting_env_vars_) return;
+
+    // Construct a new termsize from COLUMNS and LINES, then set it in our data.
+    termsize_t new_termsize{
+        var_to_int_or(vars.get(L"COLUMNS", ENV_GLOBAL), termsize_t::DEFAULT_WIDTH),
+        var_to_int_or(vars.get(L"LINES", ENV_GLOBAL), termsize_t::DEFAULT_HEIGHT),
+    };
+
+    // Store our termsize as an environment override.
+    data_.acquire()->mark_override_from_env(new_termsize);
+}
+
+// static
+void termsize_container_t::handle_winch() { s_sigwinch_gen_count += 1; }

src/termsize.h

@@ -0,0 +1,110 @@
+// Support for exposing the terminal size.
+
+#include "config.h"  // IWYU pragma: keep
+#ifndef FISH_TERMSIZE_H
+#define FISH_TERMSIZE_H
+
+#include <stdint.h>
+
+#include "common.h"
+#include "global_safety.h"
+
+class environment_t;
+class parser_t;
+struct termsize_tester_t;
+
+/// A simple value type wrapping up a terminal size.
+struct termsize_t {
+    /// Default width and height.
+    static constexpr int DEFAULT_WIDTH = 80;
+    static constexpr int DEFAULT_HEIGHT = 24;
+
+    /// width of the terminal, in columns.
+    int width{DEFAULT_WIDTH};
+
+    /// height of the terminal, in rows.
+    int height{DEFAULT_HEIGHT};
+
+    /// Construct from width and height.
+    termsize_t(int w, int h) : width(w), height(h) {}
+
+    /// Return a default-sized termsize.
+    static termsize_t defaults() { return termsize_t{DEFAULT_WIDTH, DEFAULT_HEIGHT}; }
+
+    bool operator==(termsize_t rhs) const {
+        return this->width == rhs.width && this->height == rhs.height;
+    }
+
+    bool operator!=(termsize_t rhs) const { return !(*this == rhs); }
+};
+
+/// Termsize monitoring is more complicated than one may think.
+/// The main source of complexity is the interaction between the environment variables COLUMNS/ROWS,
+/// the WINCH signal, and the TIOCGWINSZ ioctl.
+/// Our policy is "last seen wins": if COLUMNS or LINES is modified, we respect that until we get a
+/// SIGWINCH.
+struct termsize_container_t {
+    /// \return the termsize without applying any updates.
+    /// Return the default termsize if none.
+    termsize_t last() const;
+
+    /// If our termsize is stale, update it, using \p parser firing any events that may be
+    /// registered for COLUMNS and LINES.
+    /// \return the updated termsize.
+    termsize_t updating(parser_t &parser);
+
+    /// Initialize our termsize, using the given environment stack.
+    /// This will prefer to use COLUMNS and LINES, but will fall back to the tty size reader.
+    /// This does not change any variables in the environment.
+    termsize_t initialize(const environment_t &vars);
+
+    /// Note that a WINCH signal is received.
+    /// Naturally this may be called from within a signal handler.
+    static void handle_winch();
+
+    /// Note that COLUMNS and/or LINES global variables changed.
+    void handle_columns_lines_var_change(const environment_t &vars);
+
+    /// \return the singleton shared container.
+    static termsize_container_t &shared();
+
+   private:
+    /// A function used for accessing the termsize from the tty. This is only exposed for testing.
+    using tty_size_reader_func_t = maybe_t<termsize_t> (*)();
+
+    struct data_t {
+        // The last termsize returned by TIOCGWINSZ, or none if none.
+        maybe_t<termsize_t> last_from_tty{};
+
+        // The last termsize seen from the environment (COLUMNS/LINES), or none if none.
+        maybe_t<termsize_t> last_from_env{};
+
+        // The last-seen winch generation count.
+        // Set to a huge value so it's initially stale.
+        uint32_t last_winch_gen_count{UINT32_MAX};
+
+        /// \return the current termsize from this data.
+        termsize_t current() const;
+
+        /// Mark that our termsize is (for the time being) from the environment, not the tty.
+        void mark_override_from_env(termsize_t ts);
+    };
+
+    // Construct from a reader function.
+    explicit termsize_container_t(tty_size_reader_func_t func) : tty_size_reader_(func) {}
+
+    // Update COLUMNS and LINES in the parser's stack.
+    void set_columns_lines_vars(termsize_t val, parser_t &parser);
+
+    // Our lock-protected data.
+    mutable owning_lock<data_t> data_{};
+
+    // An indication that we are currently in the process of setting COLUMNS and LINES, and so do
+    // not react to any changes.
+    relaxed_atomic_bool_t setting_env_vars_{false};
+    const tty_size_reader_func_t tty_size_reader_;
+
+    friend termsize_tester_t;
+};
+
+#endif  // FISH_TERMSIZE_H