Instantize and rationalize locking around the input mapping set

Hypothetically a background thread could invoke builtin_bind, etc.
Protect the set of input bindings with a lock.

src/builtin_bind.cpp

@@ -56,7 +56,7 @@ bool builtin_bind_t::list_one(const wcstring &seq, const wcstring &bind_mode, bo
     wcstring_list_t ecmds;
     wcstring sets_mode;
 
-    if (!input_mapping_get(seq, bind_mode, &ecmds, user, &sets_mode)) {
+    if (!input_mappings_->get(seq, bind_mode, &ecmds, user, &sets_mode)) {
         return false;
     }
 
@@ -116,7 +116,7 @@ bool builtin_bind_t::list_one(const wcstring &seq, const wcstring &bind_mode, bo
 
 /// List all current key bindings.
 void builtin_bind_t::list(const wchar_t *bind_mode, bool user, io_streams_t &streams) {
-    const std::vector<input_mapping_name_t> lst = input_mapping_get_names(user);
+    const std::vector<input_mapping_name_t> lst = input_mappings_->get_names(user);
 
     for (const input_mapping_name_t &binding : lst) {
         if (bind_mode && bind_mode != binding.mode) {
@@ -180,13 +180,13 @@ bool builtin_bind_t::add(const wchar_t *seq, const wchar_t *const *cmds, size_t
     if (terminfo) {
         wcstring seq2;
         if (get_terminfo_sequence(seq, &seq2, streams)) {
-            input_mapping_add(seq2.c_str(), cmds, cmds_len, mode, sets_mode, user);
+            input_mappings_->add(seq2.c_str(), cmds, cmds_len, mode, sets_mode, user);
         } else {
             return true;
         }
 
     } else {
-        input_mapping_add(seq, cmds, cmds_len, mode, sets_mode, user);
+        input_mappings_->add(seq, cmds, cmds_len, mode, sets_mode, user);
     }
 
     return false;
@@ -207,7 +207,7 @@ bool builtin_bind_t::add(const wchar_t *seq, const wchar_t *const *cmds, size_t
 bool builtin_bind_t::erase(wchar_t **seq, bool all, const wchar_t *mode, bool use_terminfo,
                            bool user, io_streams_t &streams) {
     if (all) {
-        input_mapping_clear(mode, user);
+        input_mappings_->clear(mode, user);
         return false;
     }
 
@@ -218,12 +218,12 @@ bool builtin_bind_t::erase(wchar_t **seq, bool all, const wchar_t *mode, bool us
         if (use_terminfo) {
             wcstring seq2;
             if (get_terminfo_sequence(*seq++, &seq2, streams)) {
-                input_mapping_erase(seq2, mode, user);
+                input_mappings_->erase(seq2, mode, user);
             } else {
                 res = true;
             }
         } else {
-            input_mapping_erase(*seq++, mode, user);
+            input_mappings_->erase(*seq++, mode, user);
         }
     }
 
@@ -297,8 +297,8 @@ bool builtin_bind_t::insert(int optind, int argc, wchar_t **argv, io_streams_t &
 /// List all current bind modes.
 void builtin_bind_t::list_modes(io_streams_t &streams) {
     // List all known modes, even if they are only in preset bindings.
-    const std::vector<input_mapping_name_t> lst = input_mapping_get_names(true);
+    const std::vector<input_mapping_name_t> lst = input_mappings_->get_names(true);
-    const std::vector<input_mapping_name_t> preset_lst = input_mapping_get_names(false);
+    const std::vector<input_mapping_name_t> preset_lst = input_mappings_->get_names(false);
     // A set accomplishes two things for us here:
     // - It removes duplicates (no twenty "default" entries).
     // - It sorts it, which makes it nicer on the user.

src/builtin_bind.h

@@ -3,6 +3,7 @@
 #define FISH_BUILTIN_BIND_H
 
 #include "common.h"
+#include "input.h"
 
 class parser_t;
 struct io_streams_t;
@@ -12,9 +13,16 @@ class builtin_bind_t {
    public:
     int builtin_bind(parser_t &parser, io_streams_t &streams, wchar_t **argv);
 
+    builtin_bind_t() : input_mappings_(input_mappings()) {}
+
    private:
     bind_cmd_opts_t *opts;
 
+    /// Note that builtin_bind_t holds the singleton lock.
+    /// It must not call out to anything which can execute fish shell code or attempt to acquire the
+    /// lock again.
+    acquired_lock<input_mapping_set_t> input_mappings_;
+
     void list(const wchar_t *bind_mode, bool user, io_streams_t &streams);
     void key_names(bool all, io_streams_t &streams);
     void function_names(io_streams_t &streams);

src/fish_tests.cpp

@@ -3009,8 +3009,12 @@ static void test_input() {
     // the first!
     wcstring prefix_binding = L"qqqqqqqa";
     wcstring desired_binding = prefix_binding + L'a';
-    input_mapping_add(prefix_binding.c_str(), L"up-line");
+
-    input_mapping_add(desired_binding.c_str(), L"down-line");
+    {
+        auto input_mapping = input_mappings();
+        input_mapping->add(prefix_binding.c_str(), L"up-line");
+        input_mapping->add(desired_binding.c_str(), L"down-line");
+    }
 
     // Push the desired binding to the queue.
     for (size_t idx = 0; idx < desired_binding.size(); idx++) {

src/input.cpp

@@ -146,10 +146,14 @@ wcstring describe_char(wint_t c) {
     return format_string(L"%02x", c);
 }
 
-/// Mappings for the current input mode.
 using mapping_list_t = std::vector<input_mapping_t>;
-static mainthread_t<mapping_list_t> s_mapping_list;
+input_mapping_set_t::input_mapping_set_t() = default;
-static mainthread_t<mapping_list_t> s_preset_mapping_list;
+input_mapping_set_t::~input_mapping_set_t() = default;
+
+acquired_lock<input_mapping_set_t> input_mappings() {
+    static owning_lock<input_mapping_set_t> s_mappings{input_mapping_set_t()};
+    return s_mappings.acquire();
+}
 
 /// Terminfo map list.
 static latch_t<std::vector<terminfo_mapping_t>> s_terminfo_mappings;
@@ -200,20 +204,24 @@ static bool specification_order_is_less_than(const input_mapping_t &m1, const in
 
 /// Inserts an input mapping at the correct position. We sort them in descending order by length, so
 /// that we test longer sequences first.
-static void input_mapping_insert_sorted(const input_mapping_t &new_mapping, bool user = true) {
+static void input_mapping_insert_sorted(mapping_list_t &ml, input_mapping_t new_mapping) {
-    mapping_list_t &ml = user ? s_mapping_list : s_preset_mapping_list;
     auto loc = std::lower_bound(ml.begin(), ml.end(), new_mapping, length_is_greater_than);
-    ml.insert(loc, new_mapping);
+    ml.insert(loc, std::move(new_mapping));
 }
 
 /// Adds an input mapping.
-void input_mapping_add(const wchar_t *sequence, const wchar_t *const *commands, size_t commands_len,
+void input_mapping_set_t::add(const wchar_t *sequence, const wchar_t *const *commands,
-                       const wchar_t *mode, const wchar_t *sets_mode, bool user) {
+                              size_t commands_len, const wchar_t *mode, const wchar_t *sets_mode,
+                              bool user) {
     assert(sequence && commands && mode && sets_mode && "Null parameter");
+
+    // Clear cached mappings.
+    all_mappings_cache_.reset();
+
     // Remove existing mappings with this sequence.
     const wcstring_list_t commands_vector(commands, commands + commands_len);
 
-    mapping_list_t &ml = user ? s_mapping_list : s_preset_mapping_list;
+    mapping_list_t &ml = user ? mapping_list_ : preset_mapping_list_;
 
     for (input_mapping_t &m : ml) {
         if (m.seq == sequence && m.mode == mode) {
@@ -225,12 +233,12 @@ void input_mapping_add(const wchar_t *sequence, const wchar_t *const *commands,
 
     // Add a new mapping, using the next order.
     const input_mapping_t new_mapping = input_mapping_t(sequence, commands_vector, mode, sets_mode);
-    input_mapping_insert_sorted(new_mapping, user);
+    input_mapping_insert_sorted(ml, std::move(new_mapping));
 }
 
-void input_mapping_add(const wchar_t *sequence, const wchar_t *command, const wchar_t *mode,
+void input_mapping_set_t::add(const wchar_t *sequence, const wchar_t *command, const wchar_t *mode,
-                       const wchar_t *sets_mode, bool user) {
+                              const wchar_t *sets_mode, bool user) {
-    input_mapping_add(sequence, &command, 1, mode, sets_mode, user);
+    input_mapping_set_t::add(sequence, &command, 1, mode, sets_mode, user);
 }
 
 /// Handle interruptions to key reading by reaping finshed jobs and propagating the interrupt to the
@@ -265,23 +273,25 @@ void init_input() {
     input_common_init(&interrupt_handler);
     s_terminfo_mappings = create_input_terminfo();
 
+    auto input_mapping = input_mappings();
+
     // If we have no keybindings, add a few simple defaults.
-    mapping_list_t &preset_mapping_list = s_preset_mapping_list;
+    if (input_mapping->preset_mapping_list_.empty()) {
-    if (preset_mapping_list.empty()) {
+        input_mapping->add(L"", L"self-insert", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
-        input_mapping_add(L"", L"self-insert", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\n", L"execute", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
-        input_mapping_add(L"\n", L"execute", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\r", L"execute", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
-        input_mapping_add(L"\r", L"execute", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\t", L"complete", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
-        input_mapping_add(L"\t", L"complete", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\x3", L"commandline ''", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
-        input_mapping_add(L"\x3", L"commandline ''", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\x4", L"exit", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
-        input_mapping_add(L"\x4", L"exit", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\x5", L"bind", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
-        input_mapping_add(L"\x5", L"bind", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\x7f", L"backward-delete-char", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE,
-        input_mapping_add(L"\x7f", L"backward-delete-char", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE,
+                           false);
-                          false);
         // Arrows - can't have functions, so *-or-search isn't available.
-        input_mapping_add(L"\x1B[A", L"up-line", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\x1B[A", L"up-line", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
-        input_mapping_add(L"\x1B[B", L"down-line", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\x1B[B", L"down-line", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
-        input_mapping_add(L"\x1B[C", L"forward-char", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\x1B[C", L"forward-char", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
-        input_mapping_add(L"\x1B[D", L"backward-char", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE, false);
+        input_mapping->add(L"\x1B[D", L"backward-char", DEFAULT_BIND_MODE, DEFAULT_BIND_MODE,
+                           false);
     }
 }
 
@@ -411,32 +421,28 @@ void inputter_t::push_front(char_event_t ch) { event_queue_.push_front(ch); }
 
 /// \return the first mapping that matches, walking first over the user's mapping list, then the
 /// preset list. \return null if nothing matches.
-const input_mapping_t *inputter_t::find_mapping() {
+maybe_t<input_mapping_t> inputter_t::find_mapping() {
     const input_mapping_t *generic = NULL;
     const auto &vars = parser_->vars();
     const wcstring bind_mode = input_get_bind_mode(vars);
 
-    const auto lists = {&s_mapping_list, &s_preset_mapping_list};
+    auto ml = input_mappings()->all_mappings();
-    for (const auto *listp : lists) {
+    for (const auto &m : *ml) {
-        const mapping_list_t &ml = *listp;
+        if (m.mode != bind_mode) {
-        for (const auto &m : ml) {
+            continue;
-            if (m.mode != bind_mode) {
+        }
-                continue;
-            }
 
-            if (m.is_generic()) {
+        if (m.is_generic()) {
-                if (!generic) generic = &m;
+            if (!generic) generic = &m;
-            } else if (mapping_is_match(m)) {
+        } else if (mapping_is_match(m)) {
-                return &m;
+            return m;
-            }
         }
     }
-    return generic;
+    return generic ? maybe_t<input_mapping_t>(*generic) : none();
 }
 
 void inputter_t::mapping_execute_matching_or_generic(bool allow_commands) {
-    const input_mapping_t *mapping = find_mapping();
+    if (auto mapping = find_mapping()) {
-    if (mapping) {
         mapping_execute(*mapping, allow_commands);
     } else {
         debug(2, L"no generic found, ignoring char...");
@@ -510,10 +516,10 @@ char_event_t inputter_t::readch(bool allow_commands) {
     }
 }
 
-std::vector<input_mapping_name_t> input_mapping_get_names(bool user) {
+std::vector<input_mapping_name_t> input_mapping_set_t::get_names(bool user) const {
     // Sort the mappings by the user specification order, so we can return them in the same order
     // that the user specified them in.
-    std::vector<input_mapping_t> local_list = user ? s_mapping_list : s_preset_mapping_list;
+    std::vector<input_mapping_t> local_list = user ? mapping_list_ : preset_mapping_list_;
     std::sort(local_list.begin(), local_list.end(), specification_order_is_less_than);
     std::vector<input_mapping_name_t> result;
     result.reserve(local_list.size());
@@ -525,17 +531,19 @@ std::vector<input_mapping_name_t> input_mapping_get_names(bool user) {
     return result;
 }
 
-void input_mapping_clear(const wchar_t *mode, bool user) {
+void input_mapping_set_t::clear(const wchar_t *mode, bool user) {
-    ASSERT_IS_MAIN_THREAD();
+    all_mappings_cache_.reset();
-    mapping_list_t &ml = user ? s_mapping_list : s_preset_mapping_list;
+    mapping_list_t &ml = user ? mapping_list_ : preset_mapping_list_;
     auto should_erase = [=](const input_mapping_t &m) { return mode == NULL || mode == m.mode; };
     ml.erase(std::remove_if(ml.begin(), ml.end(), should_erase), ml.end());
 }
 
-bool input_mapping_erase(const wcstring &sequence, const wcstring &mode, bool user) {
+bool input_mapping_set_t::erase(const wcstring &sequence, const wcstring &mode, bool user) {
-    ASSERT_IS_MAIN_THREAD();
+    // Clear cached mappings.
+    all_mappings_cache_.reset();
+
     bool result = false;
-    mapping_list_t &ml = user ? s_mapping_list : s_preset_mapping_list;
+    mapping_list_t &ml = user ? mapping_list_ : preset_mapping_list_;
     for (std::vector<input_mapping_t>::iterator it = ml.begin(), end = ml.end(); it != end; ++it) {
         if (sequence == it->seq && mode == it->mode) {
             ml.erase(it);
@@ -546,10 +554,10 @@ bool input_mapping_erase(const wcstring &sequence, const wcstring &mode, bool us
     return result;
 }
 
-bool input_mapping_get(const wcstring &sequence, const wcstring &mode, wcstring_list_t *out_cmds,
+bool input_mapping_set_t::get(const wcstring &sequence, const wcstring &mode,
-                       bool user, wcstring *out_sets_mode) {
+                              wcstring_list_t *out_cmds, bool user, wcstring *out_sets_mode) {
     bool result = false;
-    mapping_list_t &ml = user ? s_mapping_list : s_preset_mapping_list;
+    mapping_list_t &ml = user ? mapping_list_ : preset_mapping_list_;
     for (const input_mapping_t &m : ml) {
         if (sequence == m.seq && mode == m.mode) {
             *out_cmds = m.commands;
@@ -561,6 +569,17 @@ bool input_mapping_get(const wcstring &sequence, const wcstring &mode, wcstring_
     return result;
 }
 
+std::shared_ptr<const mapping_list_t> input_mapping_set_t::all_mappings() {
+    // Populate the cache if needed.
+    if (!all_mappings_cache_) {
+        mapping_list_t all_mappings = mapping_list_;
+        all_mappings.insert(all_mappings.end(), preset_mapping_list_.begin(),
+                            preset_mapping_list_.end());
+        all_mappings_cache_ = std::make_shared<const mapping_list_t>(std::move(all_mappings));
+    }
+    return all_mappings_cache_;
+}
+
 /// Create a list of terminfo mappings.
 static std::vector<terminfo_mapping_t> create_input_terminfo() {
     assert(curses_initialized);

src/input.h

@@ -35,7 +35,7 @@ class inputter_t {
     void mapping_execute(const input_mapping_t &m, bool allow_commands);
     void mapping_execute_matching_or_generic(bool allow_commands);
     bool mapping_is_match(const input_mapping_t &m);
-    const input_mapping_t *find_mapping();
+    maybe_t<input_mapping_t> find_mapping();
     char_event_t read_characters_no_readline();
 
    public:
@@ -69,40 +69,60 @@ class inputter_t {
     wchar_t function_pop_arg();
 };
 
-/// Add a key mapping from the specified sequence to the specified command.
-///
-/// \param sequence the sequence to bind
-/// \param command an input function that will be run whenever the key sequence occurs
-void input_mapping_add(const wchar_t *sequence, const wchar_t *command,
-                       const wchar_t *mode = DEFAULT_BIND_MODE,
-                       const wchar_t *new_mode = DEFAULT_BIND_MODE, bool user = true);
-
-void input_mapping_add(const wchar_t *sequence, const wchar_t *const *commands, size_t commands_len,
-                       const wchar_t *mode = DEFAULT_BIND_MODE,
-                       const wchar_t *new_mode = DEFAULT_BIND_MODE, bool user = true);
-
 struct input_mapping_name_t {
     wcstring seq;
     wcstring mode;
 };
 
-/// Returns all mapping names and modes.
+/// The input mapping set is the set of mappings from character sequences to commands.
-std::vector<input_mapping_name_t> input_mapping_get_names(bool user = true);
+class input_mapping_set_t {
+    friend acquired_lock<input_mapping_set_t> input_mappings();
+    friend void init_input();
 
-/// Erase all bindings
+    using mapping_list_t = std::vector<input_mapping_t>;
-void input_mapping_clear(const wchar_t *mode = NULL, bool user = true);
 
-/// Erase binding for specified key sequence.
+    mapping_list_t mapping_list_;
-bool input_mapping_erase(const wcstring &sequence, const wcstring &mode = DEFAULT_BIND_MODE,
+    mapping_list_t preset_mapping_list_;
-                         bool user = true);
+    std::shared_ptr<const mapping_list_t> all_mappings_cache_;
 
-/// Gets the command bound to the specified key sequence in the specified mode. Returns true if it
+    input_mapping_set_t();
-/// exists, false if not.
-bool input_mapping_get(const wcstring &sequence, const wcstring &mode, wcstring_list_t *out_cmds,
-                       bool user, wcstring *out_new_mode);
 
-/// Sets the return status of the most recently executed input function.
+   public:
-void input_function_set_status(bool status);
+    ~input_mapping_set_t();
+
+    /// Erase all bindings.
+    void clear(const wchar_t *mode = NULL, bool user = true);
+
+    /// Erase binding for specified key sequence.
+    bool erase(const wcstring &sequence, const wcstring &mode = DEFAULT_BIND_MODE,
+               bool user = true);
+
+    /// Gets the command bound to the specified key sequence in the specified mode. Returns true if
+    /// it exists, false if not.
+    bool get(const wcstring &sequence, const wcstring &mode, wcstring_list_t *out_cmds, bool user,
+             wcstring *out_new_mode);
+
+    /// Returns all mapping names and modes.
+    std::vector<input_mapping_name_t> get_names(bool user = true) const;
+
+    /// Add a key mapping from the specified sequence to the specified command.
+    ///
+    /// \param sequence the sequence to bind
+    /// \param command an input function that will be run whenever the key sequence occurs
+    void add(const wchar_t *sequence, const wchar_t *command,
+             const wchar_t *mode = DEFAULT_BIND_MODE, const wchar_t *new_mode = DEFAULT_BIND_MODE,
+             bool user = true);
+
+    void add(const wchar_t *sequence, const wchar_t *const *commands, size_t commands_len,
+             const wchar_t *mode = DEFAULT_BIND_MODE, const wchar_t *new_mode = DEFAULT_BIND_MODE,
+             bool user = true);
+
+    /// \return a snapshot of the list of input mappings.
+    std::shared_ptr<const mapping_list_t> all_mappings();
+};
+
+/// Access the singleton input mapping set.
+acquired_lock<input_mapping_set_t> input_mappings();
 
 /// Return the sequence for the terminfo variable of the specified name.
 ///