Make commandline state thread safe

Today the reader exposes its internals directly, e.g. to the commandline
builtin. This is of course not thread safe. For example in concurrent
execution, running `commandline` twice in separate threads would cause a
race and likely a crash.

Fix this by factoring all the commandline state into a new type
'commandline_state_t'. Make it a singleton (there is only one command
line
after all) and protect it with a lock.

No user visible change here.

src/builtin_commandline.cpp

@@ -80,7 +80,7 @@ static void replace_part(const wchar_t *begin, const wchar_t *end, const wchar_t
         }
     }
     out.append(end);
-    reader_set_buffer(out, out_pos);
+    commandline_set_buffer(out, out_pos);
 }
 
 /// Output the specified selection.
@@ -125,13 +125,7 @@ static void write_part(const wchar_t *begin, const wchar_t *end, int cut_at_curs
 
 /// The commandline builtin. It is used for specifying a new value for the commandline.
 maybe_t<int> builtin_commandline(parser_t &parser, io_streams_t &streams, const wchar_t **argv) {
-    // Pointer to what the commandline builtin considers to be the current contents of the command
-    // line buffer.
-    const wchar_t *current_buffer = nullptr;
-
-    // What the commandline builtin considers to be the current cursor position.
-    auto current_cursor_pos = static_cast<size_t>(-1);
-
+    const commandline_state_t rstate = commandline_get_state();
     const wchar_t *cmd = argv[0];
     int buffer_part = 0;
     bool cut_at_cursor = false;
@@ -149,30 +143,9 @@ maybe_t<int> builtin_commandline(parser_t &parser, io_streams_t &streams, const
     bool search_mode = false;
     bool paging_mode = false;
     const wchar_t *begin = nullptr, *end = nullptr;
+    const wchar_t *override_buffer = nullptr;
 
     const auto &ld = parser.libdata();
-    wcstring transient_commandline;
-    if (!ld.transient_commandlines.empty()) {
-        transient_commandline = ld.transient_commandlines.back();
-        current_buffer = transient_commandline.c_str();
-        current_cursor_pos = transient_commandline.size();
-    } else {
-        current_buffer = reader_get_buffer();
-        current_cursor_pos = reader_get_cursor_pos();
-    }
-
-    if (!current_buffer) {
-        if (is_interactive_session()) {
-            // Prompt change requested while we don't have a prompt, most probably while reading the
-            // init files. Just ignore it.
-            return STATUS_CMD_ERROR;
-        }
-
-        streams.err.append(argv[0]);
-        streams.err.append(L": Can not set commandline in non-interactive mode\n");
-        builtin_print_error_trailer(parser, streams.err, cmd);
-        return STATUS_CMD_ERROR;
-    }
 
     static const wchar_t *const short_options = L":abijpctforhI:CLSsP";
     static const struct woption long_options[] = {{L"append", no_argument, nullptr, 'a'},
@@ -239,8 +212,7 @@ maybe_t<int> builtin_commandline(parser_t &parser, io_streams_t &streams, const
                 break;
             }
             case 'I': {
-                current_buffer = w.woptarg;
-                current_cursor_pos = std::wcslen(w.woptarg);
+                override_buffer = w.woptarg;
                 break;
             }
             case 'C': {
@@ -328,10 +300,9 @@ maybe_t<int> builtin_commandline(parser_t &parser, io_streams_t &streams, const
     }
 
     if (selection_mode) {
-        size_t start, len;
-        const wchar_t *buffer = reader_get_buffer();
-        if (reader_get_selection(&start, &len)) {
-            streams.out.append(buffer + start, len);
+        if (rstate.selection) {
+            streams.out.append(rstate.text.c_str() + rstate.selection->start,
+                               rstate.selection->length);
         }
         return STATUS_CMD_OK;
     }
@@ -375,19 +346,42 @@ maybe_t<int> builtin_commandline(parser_t &parser, io_streams_t &streams, const
     }
 
     if (line_mode) {
-        size_t pos = reader_get_cursor_pos();
-        const wchar_t *buff = reader_get_buffer();
-        streams.out.append_format(L"%lu\n",
-                                  static_cast<unsigned long>(parse_util_lineno(buff, pos)));
+        streams.out.append_format(L"%d\n",
+                                  parse_util_lineno(rstate.text.c_str(), rstate.cursor_pos));
         return STATUS_CMD_OK;
     }
 
     if (search_mode) {
-        return reader_is_in_search_mode() ? 0 : 1;
+        return commandline_get_state().search_mode ? 0 : 1;
     }
 
     if (paging_mode) {
-        return reader_has_pager_contents() ? 0 : 1;
+        return commandline_get_state().pager_mode ? 0 : 1;
+    }
+
+    // At this point we have (nearly) exhausted the options which always operate on the true command
+    // line. Now we respect the possibility of a transient command line due to evaluating a wrapped
+    // completion. Don't do this in cursor_mode: it makes no sense to move the cursor based on a
+    // transient commandline.
+    const wchar_t *current_buffer = nullptr;
+    size_t current_cursor_pos{0};
+    wcstring transient;
+    if (!ld.transient_commandlines.empty() && !cursor_mode) {
+        transient = ld.transient_commandlines.back();
+        current_buffer = transient.c_str();
+        current_cursor_pos = transient.size();
+    } else if (rstate.initialized) {
+        current_buffer = rstate.text.c_str();
+        current_cursor_pos = rstate.cursor_pos;
+    } else {
+        // There is no command line, either because we are not interactive, or because we are
+        // interactive and are still reading init files (in which case we silently ignore this).
+        if (!is_interactive_session()) {
+            streams.err.append(cmd);
+            streams.err.append(L": Can not set commandline in non-interactive mode\n");
+            builtin_print_error_trailer(parser, streams.err, cmd);
+        }
+        return STATUS_CMD_ERROR;
     }
 
     switch (buffer_part) {
@@ -422,12 +416,11 @@ maybe_t<int> builtin_commandline(parser_t &parser, io_streams_t &streams, const
                 builtin_print_error_trailer(parser, streams.err, cmd);
             }
 
-            current_buffer = reader_get_buffer();
             new_pos =
                 std::max(0L, std::min(new_pos, static_cast<long>(std::wcslen(current_buffer))));
-            reader_set_buffer(current_buffer, static_cast<size_t>(new_pos));
+            commandline_set_buffer(current_buffer, static_cast<size_t>(new_pos));
         } else {
-            size_t pos = reader_get_cursor_pos() - (begin - current_buffer);
+            size_t pos = current_cursor_pos - (begin - current_buffer);
             streams.out.append_format(L"%lu\n", static_cast<unsigned long>(pos));
         }
         return STATUS_CMD_OK;

src/builtin_complete.cpp

@@ -342,14 +342,14 @@ maybe_t<int> builtin_complete(parser_t &parser, io_streams_t &streams, const wch
     if (do_complete) {
         if (!have_do_complete_param) {
             // No argument given, try to use the current commandline.
-            const wchar_t *cmd = reader_get_buffer();
-            if (cmd == nullptr) {
+            auto state = commandline_get_state();
+            if (!state.initialized) {
                 // This corresponds to using 'complete -C' in non-interactive mode.
                 // See #2361    .
                 builtin_missing_argument(parser, streams, cmd, argv[w.woptind - 1]);
                 return STATUS_INVALID_ARGS;
             }
-            do_complete_param = cmd;
+            do_complete_param = std::move(state.text);
         }
         const wchar_t *token;
 

src/builtin_history.cpp

@@ -216,7 +216,7 @@ maybe_t<int> builtin_history(parser_t &parser, io_streams_t &streams, const wcha
 
     // Use the default history if we have none (which happens if invoked non-interactively, e.g.
     // from webconfig.py.
-    std::shared_ptr<history_t> history = reader_get_history();
+    std::shared_ptr<history_t> history = commandline_get_state().history;
     if (!history) history = history_t::with_name(history_session_id(parser.vars()));
 
     // If a history command hasn't already been specified via a flag check the first word.

src/builtin_read.cpp

@@ -223,7 +223,7 @@ static int read_interactive(parser_t &parser, wcstring &buff, int nchars, bool s
     // Keep in-memory history only.
     reader_push(parser, wcstring{}, std::move(conf));
 
-    reader_set_buffer(commandline, std::wcslen(commandline));
+    commandline_set_buffer(commandline, std::wcslen(commandline));
     scoped_push<bool> interactive{&parser.libdata().is_interactive, true};
 
     event_fire_generic(parser, L"fish_read");

src/env.cpp

@@ -729,7 +729,7 @@ maybe_t<env_var_t> env_scoped_impl_t::try_get_computed(const wcstring &key) cons
             return none();
         }
 
-        std::shared_ptr<history_t> history = reader_get_history();
+        std::shared_ptr<history_t> history = commandline_get_state().history;
         if (!history) {
             history = history_t::with_name(history_session_id(*this));
         }

src/reader.cpp

@@ -123,6 +123,22 @@ enum class history_search_direction_t { forward, backward };
 enum class jump_direction_t { forward, backward };
 enum class jump_precision_t { till, to };
 
+/// A singleton snapshot of the reader state. This is updated when the reader changes. This is
+/// factored out for thread-safety reasons: it may be fetched on a background thread.
+static acquired_lock<commandline_state_t> commandline_state_snapshot() {
+    // Deliberately leaked to avoid shutdown dtors.
+    static owning_lock<commandline_state_t> *const s_state = new owning_lock<commandline_state_t>();
+    return s_state->acquire();
+}
+
+commandline_state_t commandline_get_state() { return *commandline_state_snapshot(); }
+
+void commandline_set_buffer(wcstring text, size_t cursor_pos) {
+    auto state = commandline_state_snapshot();
+    state->cursor_pos = std::min(cursor_pos, text.size());
+    state->text = std::move(text);
+}
+
 /// Any time the contents of a buffer changes, we update the generation count. This allows for our
 /// background threads to notice it and skip doing work that they would otherwise have to do.
 static std::atomic<uint32_t> s_generation;
@@ -715,6 +731,15 @@ class reader_data_t : public std::enable_shared_from_this<reader_data_t> {
     /// \return the command, or none if we were asked to cancel (e.g. SIGHUP).
     maybe_t<wcstring> readline(int nchars);
 
+    /// Reflect our current data in the command line state snapshot.
+    /// This is called before we run any fish script, so that the commandline builtin can see our
+    /// state.
+    void update_commandline_state() const;
+
+    /// Apply any changes from the reader snapshot. This is called after running fish script,
+    /// incorporating changes from the commandline builtin.
+    void apply_commandline_state_changes();
+
     void move_word(editable_line_t *el, bool move_right, bool erase, enum move_word_style_t style,
                    bool newv);
 
@@ -725,6 +750,8 @@ class reader_data_t : public std::enable_shared_from_this<reader_data_t> {
     void select_completion_in_direction(selection_motion_t dir);
     void flash();
 
+    maybe_t<source_range_t> get_selection() const;
+
     void completion_insert(const wcstring &val, size_t token_end, complete_flags_t flags);
 
     bool can_autosuggest() const;
@@ -1103,6 +1130,8 @@ void reader_data_t::command_line_changed(const editable_line_t *el) {
         this->pager.refilter_completions();
         this->pager_selection_changed();
     }
+    // Ensure that the commandline builtin sees our new state.
+    update_commandline_state();
 }
 
 void reader_data_t::pager_selection_changed() {
@@ -1849,6 +1878,14 @@ void reader_data_t::flash() {
     paint_layout(L"unflash");
 }
 
+maybe_t<source_range_t> reader_data_t::get_selection() const {
+    if (!this->selection.has_value()) return none();
+    size_t start = this->selection->start;
+    size_t len =
+        std::min(this->selection->stop, this->command_line.size()) - this->selection->start;
+    return source_range_t{static_cast<uint32_t>(start), static_cast<uint32_t>(len)};
+}
+
 /// Characters that may not be part of a token that is to be replaced by a case insensitive
 /// completion.
 #define REPLACE_UNCLEAN L"$*?({})"
@@ -2559,6 +2596,7 @@ void reader_change_history(const wcstring &name) {
     if (data && data->history) {
         data->history->save();
         data->history = history_t::with_name(name);
+        commandline_state_snapshot()->history = data->history;
     }
 }
 
@@ -2575,6 +2613,7 @@ static std::shared_ptr<reader_data_t> reader_push_ret(parser_t &parser,
     if (reader_data_stack.size() == 1) {
         reader_interactive_init(parser);
     }
+    data->update_commandline_state();
     return data;
 }
 
@@ -2589,8 +2628,10 @@ void reader_pop() {
     reader_data_t *new_reader = current_data_or_null();
     if (new_reader == nullptr) {
         reader_interactive_destroy();
+        *commandline_state_snapshot() = commandline_state_t{};
     } else {
         s_reset_abandoning_line(&new_reader->screen, termsize_last().width);
+        new_reader->update_commandline_state();
     }
 }
 
@@ -2661,6 +2702,29 @@ static bool selection_is_at_top(const reader_data_t *data) {
     return !(col != 0 && col != PAGER_SELECTION_NONE);
 }
 
+void reader_data_t::update_commandline_state() const {
+    auto snapshot = commandline_state_snapshot();
+    snapshot->text = this->command_line.text();
+    snapshot->cursor_pos = this->command_line.position();
+    snapshot->history = this->history;
+    snapshot->selection = this->get_selection();
+    snapshot->pager_mode = !this->current_page_rendering.screen_data.empty();
+    snapshot->search_mode = this->history_search.active();
+    snapshot->initialized = true;
+}
+
+void reader_data_t::apply_commandline_state_changes() {
+    // Only the text and cursor position may be changed.
+    commandline_state_t state = *commandline_state_snapshot();
+    if (state.text != this->command_line.text() ||
+        state.cursor_pos != this->command_line.position()) {
+        // The commandline builtin changed our contents.
+        this->pager.clear();
+        this->set_buffer_maintaining_pager(state.text, state.cursor_pos);
+        this->reset_loop_state = true;
+    }
+}
+
 static relaxed_atomic_t<uint64_t> run_count{0};
 
 /// Returns the current interactive loop count
@@ -2820,11 +2884,19 @@ struct readline_loop_state_t {
     size_t nchars{std::numeric_limits<size_t>::max()};
 };
 
+std::shared_ptr<history_t> reader_get_history() {
+    ASSERT_IS_MAIN_THREAD();
+    reader_data_t *data = current_data_or_null();
+    return data ? data->history : nullptr;
+}
+
 /// Run a sequence of commands from an input binding.
 void reader_data_t::run_input_command_scripts(const wcstring_list_t &cmds) {
     auto last_statuses = parser().get_last_statuses();
     for (const wcstring &cmd : cmds) {
+        update_commandline_state();
         parser().eval(cmd, io_chain_t{});
+        apply_commandline_state_changes();
     }
     parser().set_last_statuses(std::move(last_statuses));
 
@@ -3623,8 +3695,10 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
             editable_line_t *el = active_edit_line();
 
             // Check that we have an active selection and get the bounds.
-            size_t start, len;
-            if (reader_get_selection(&start, &len)) {
+            if (auto selection = this->get_selection()) {
+                size_t start = selection->start;
+                size_t len = selection->length;
+
                 size_t buff_pos = el->position();
                 wcstring replacement;
 
@@ -3714,9 +3788,8 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
 
         case rl::kill_selection: {
             bool newv = (rls.last_cmd != rl::kill_selection);
-            size_t start, len;
-            if (reader_get_selection(&start, &len)) {
-                kill(&command_line, start, len, KILL_APPEND, newv);
+            if (auto selection = this->get_selection()) {
+                kill(&command_line, selection->start, selection->length, KILL_APPEND, newv);
             }
             break;
         }
@@ -4089,15 +4162,6 @@ bool reader_data_t::jump(jump_direction_t dir, jump_precision_t precision, edita
 
 maybe_t<wcstring> reader_readline(int nchars) { return current_data()->readline(nchars); }
 
-bool reader_is_in_search_mode() {
-    reader_data_t *data = current_data_or_null();
-    return data && data->history_search.active();
-}
-
-bool reader_has_pager_contents() {
-    reader_data_t *data = current_data_or_null();
-    return data && !data->current_page_rendering.screen_data.empty();
-}
 
 int reader_reading_interrupted() {
     int res = reader_test_and_clear_interrupted();
@@ -4132,18 +4196,6 @@ void reader_queue_ch(const char_event_t &ch) {
     }
 }
 
-const wchar_t *reader_get_buffer() {
-    ASSERT_IS_MAIN_THREAD();
-    reader_data_t *data = current_data_or_null();
-    return data ? data->command_line.text().c_str() : nullptr;
-}
-
-std::shared_ptr<history_t> reader_get_history() {
-    ASSERT_IS_MAIN_THREAD();
-    reader_data_t *data = current_data_or_null();
-    return data ? data->history : nullptr;
-}
-
 /// Sets the command line contents, clearing the pager.
 void reader_set_buffer(const wcstring &b, size_t pos) {
     reader_data_t *data = current_data_or_null();
@@ -4154,23 +4206,6 @@ void reader_set_buffer(const wcstring &b, size_t pos) {
     data->reset_loop_state = true;
 }
 
-size_t reader_get_cursor_pos() {
-    reader_data_t *data = current_data_or_null();
-    if (!data) return static_cast<size_t>(-1);
-
-    return data->command_line.position();
-}
-
-bool reader_get_selection(size_t *start, size_t *len) {
-    bool result = false;
-    reader_data_t *data = current_data_or_null();
-    if (data != nullptr && data->selection.has_value()) {
-        *start = data->selection->start;
-        *len = std::min(data->selection->stop, data->command_line.size()) - data->selection->start;
-        result = true;
-    }
-    return result;
-}
 
 /// Read non-interactively.  Read input from stdin without displaying the prompt, using syntax
 /// highlighting. This is used for reading scripts and init files.

src/reader.h

@@ -165,28 +165,6 @@ void reader_schedule_prompt_repaint();
 class char_event_t;
 void reader_queue_ch(const char_event_t &ch);
 
-/// Get the string of character currently entered into the command buffer, or 0 if interactive mode
-/// is uninitialized.
-const wchar_t *reader_get_buffer();
-
-/// Returns the current reader's history.
-std::shared_ptr<history_t> reader_get_history();
-
-/// Set the string of characters in the command buffer, as well as the cursor position.
-///
-/// \param b the new buffer value
-/// \param p the cursor position. If \c p is larger than the length of the command line, the cursor
-/// is placed on the last character.
-void reader_set_buffer(const wcstring &b, size_t p = -1);
-
-/// Get the current cursor position in the command line. If interactive mode is uninitialized,
-/// return (size_t)-1.
-size_t reader_get_cursor_pos();
-
-/// Get the current selection range in the command line. Returns false if there is no active
-/// selection, true otherwise.
-bool reader_get_selection(size_t *start, size_t *len);
-
 /// Return the value of the interrupted flag, which is set by the sigint handler, and clear it if it
 /// was set. In practice this will return 0 or SIGINT.
 int reader_test_and_clear_interrupted();
@@ -254,12 +232,6 @@ void reader_handle_sigint();
 /// TODO: this doesn't belong in reader.
 bool check_cancel_from_fish_signal();
 
-/// Test whether the interactive reader is in search mode.
-bool reader_is_in_search_mode();
-
-/// Test whether the interactive reader has visible pager contents.
-bool reader_has_pager_contents();
-
 /// Given a command line and an autosuggestion, return the string that gets shown to the user.
 /// Exposed for testing purposes only.
 wcstring combine_command_and_autosuggestion(const wcstring &cmdline,
@@ -275,6 +247,24 @@ wcstring completion_apply_to_command_line(const wcstring &val_str, complete_flag
                                           const wcstring &command_line, size_t *inout_cursor_pos,
                                           bool append_only);
 
+/// Snapshotted state from the reader.
+struct commandline_state_t {
+    wcstring text;                         // command line text, or empty if not interactive
+    size_t cursor_pos{0};                  // position of the cursor, may be as large as text.size()
+    maybe_t<source_range_t> selection{};   // visual selection, or none if none
+    std::shared_ptr<history_t> history{};  // current reader history, or null if not interactive
+    bool pager_mode{false};                // pager is visible
+    bool search_mode{false};               // pager is visible and search is active
+    bool initialized{false};               // if false, the reader has not yet been entered
+};
+
+/// Get the command line state. This may be fetched on a background thread.
+commandline_state_t commandline_get_state();
+
+/// Set the command line text and position. This may be called on a background thread; the reader
+/// will pick it up when it is done executing.
+void commandline_set_buffer(wcstring text, size_t cursor_pos = -1);
+
 /// Return the current interactive reads loop count. Useful for determining how many commands have
 /// been executed between invocations of code.
 uint64_t reader_run_count();

tests/pexpects/commandline.py

@@ -30,3 +30,9 @@ expect_prompt()
 
 sendline("echo foo")
 expect_prompt("foo")
+
+# commandline is empty when a command is executed.
+sendline("set what (commandline)")
+expect_prompt()
+sendline('echo "<$what>"')
+expect_prompt("<>")