Reimplement the function store and autoloading

This cleans up how functions are stored and autoloaded. It eliminates the
recursive lock. Instead there is a single normal owning_lock that protects
the entirety of the function data. Autoloading is re-implemented via the
new autoloader_t.

src/autoload.cpp

@@ -198,6 +198,12 @@ file_access_attempt_t access_file(const wcstring &path, int mode) {
     return result;
 }
 
+void autoloader_t::perform_autoload(const wcstring &path) {
+    wcstring script_source = L"source " + escape_string(path, ESCAPE_ALL);
+    exec_subshell(script_source, parser_t::principal_parser(),
+                  false /* do not apply exit status */);
+}
+
 autoload_t::autoload_t(wcstring env_var_name_var) : env_var_name(std::move(env_var_name_var)) {}
 
 void autoload_t::entry_was_evicted(wcstring key, autoload_function_t node) {

src/autoload.h

@@ -54,6 +54,11 @@ class autoloader_t {
     /// code; it is the caller's responsibility to load the file.
     maybe_t<wcstring> resolve_command(const wcstring &cmd, const environment_t &env);
 
+    /// Helper to actually perform an autoload.
+    /// This is a static function because it executes fish script, and so must be called without
+    /// holding any particular locks.
+    static void perform_autoload(const wcstring &path);
+
     /// Mark that a command previously returned from path_to_autoload is finished autoloading.
     void mark_autoload_finished(const wcstring &cmd) {
         size_t amt = current_autoloading_.erase(cmd);

src/function.cpp

@@ -22,6 +22,7 @@
 #include "common.h"
 #include "env.h"
 #include "event.h"
+#include "exec.h"
 #include "fallback.h"  // IWYU pragma: keep
 #include "function.h"
 #include "intern.h"
@@ -31,65 +32,89 @@
 #include "wutil.h"  // IWYU pragma: keep
 
 class function_info_t {
-public:
+   public:
- /// Immutable properties of the function.
+    /// Immutable properties of the function.
- std::shared_ptr<const function_properties_t> props;
+    std::shared_ptr<const function_properties_t> props;
- /// Function description. This may be changed after the function is created.
+    /// Function description. This may be changed after the function is created.
- wcstring description;
+    wcstring description;
- /// File where this function was defined (intern'd string).
+    /// File where this function was defined (intern'd string).
- const wchar_t *const definition_file;
+    const wchar_t *const definition_file;
- /// Mapping of all variables that were inherited from the function definition scope to their
+    /// Mapping of all variables that were inherited from the function definition scope to their
- /// values.
+    /// values.
- const std::map<wcstring, env_var_t> inherit_vars;
+    const std::map<wcstring, env_var_t> inherit_vars;
- /// Flag for specifying that this function was automatically loaded.
+    /// Flag for specifying that this function was automatically loaded.
- const bool is_autoload;
+    const bool is_autoload;
 
- /// Constructs relevant information from the function_data.
+    /// Constructs relevant information from the function_data.
- function_info_t(function_data_t data, const environment_t &vars, const wchar_t *filename,
+    function_info_t(function_data_t data, const environment_t &vars, const wchar_t *filename,
-                 bool autoload);
+                    bool autoload);
 
- /// Used by function_copy.
+    /// Used by function_copy.
- function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload);
+    function_info_t(const function_info_t &data, const wchar_t *filename, bool autoload);
 };
 
-/// Table containing all functions.
+/// Type wrapping up the set of all functions.
-typedef std::unordered_map<wcstring, function_info_t> function_map_t;
+/// There's only one of these; it's managed by a lock.
-static function_map_t loaded_functions;
+struct function_set_t {
+    /// The map of all functions by name.
+    std::unordered_map<wcstring, function_info_t> funcs;
 
-/// Functions that shouldn't be autoloaded (anymore).
+    /// Tombstones for functions that should no longer be autoloaded.
-static std::unordered_set<wcstring> function_tombstones;
+    std::unordered_set<wcstring> autoload_tombstones;
 
-/// Lock for functions.
+    /// The autoloader for our functions.
-static std::recursive_mutex functions_lock;
+    autoloader_t autoloader{L"fish_function_path"};
 
-// Function autoloader
+    /// Remove a function.
-static autoload_t function_autoloader(L"fish_function_path");
+    /// \return true if successful, false if it doesn't exist.
+    bool remove(const wcstring &name);
 
-/// Kludgy flag set by the load function in order to tell function_add that the function being
+    /// Get the info for a function, or nullptr if none.
-/// defined is autoloaded. There should be a better way to do this...
+    const function_info_t *get_info(const wcstring &name) const {
-static bool is_autoload = false;
+        auto iter = funcs.find(name);
+        return iter == funcs.end() ? nullptr : &iter->second;
+    }
+
+    /// \return true if we should allow autoloading a given function.
+    bool allow_autoload(const wcstring &name) const;
+
+    function_set_t() = default;
+};
+
+/// The big set of all functions.
+static owning_lock<function_set_t> function_set;
+
+bool function_set_t::allow_autoload(const wcstring &name) const {
+    // Prohibit autoloading if we have a non-autoload (explicit) function, or if the function is
+    // tombstoned.
+    const auto *info = get_info(name);
+    bool has_explicit_func = info && !info->is_autoload;
+    bool is_tombstoned = autoload_tombstones.count(name) > 0;
+    return !has_explicit_func && !is_tombstoned;
+}
 
 /// Make sure that if the specified function is a dynamically loaded function, it has been fully
 /// loaded.
-static int load(const wcstring &name) {
+/// Note this executes fish script code.
+static void try_autoload(const wcstring &name) {
     ASSERT_IS_MAIN_THREAD();
-    scoped_rlock locker(functions_lock);
+    maybe_t<wcstring> path_to_autoload;
-    bool was_autoload = is_autoload;
+    // Note we can't autoload while holding the funcset lock.
-    int res;
+    // Lock around a local region.
-
+    {
-    bool no_more_autoload = function_tombstones.count(name) > 0;
+        auto funcset = function_set.acquire();
-    if (no_more_autoload) return 0;
+        if (funcset->allow_autoload(name)) {
-
+            const environment_t &vars = parser_t::principal_parser().vars();
-    function_map_t::iterator iter = loaded_functions.find(name);
+            path_to_autoload = funcset->autoloader.resolve_command(name, vars);
-    if (iter != loaded_functions.end() && !iter->second.is_autoload) {
+        }
-        // We have a non-autoload version already.
-        return 0;
     }
 
-    is_autoload = true;
+    // Release the lock and perform any autoload, then reacquire the lock and clean up.
-    res = function_autoloader.load(name, true);
+    if (path_to_autoload) {
-    is_autoload = was_autoload;
+        // Crucially, the lock is acquired *after* do_autoload_file_at_path().
-    return res;
+        autoloader_t::perform_autoload(*path_to_autoload);
+        function_set.acquire()->autoloader.mark_autoload_finished(name);
+    }
 }
 
 /// Insert a list of all dynamically loaded functions into the specified list.
@@ -151,21 +176,26 @@ function_info_t::function_info_t(const function_info_t &data, const wchar_t *fil
       is_autoload(autoload) {}
 
 void function_add(const function_data_t &data, const parser_t &parser) {
-    UNUSED(parser);
     ASSERT_IS_MAIN_THREAD();
+    auto funcset = function_set.acquire();
 
-    CHECK(!data.name.empty(), );  //!OCLINT(multiple unary operator)
+    // Historical check. TODO: rationalize this.
-    scoped_rlock locker(functions_lock);
+    if (data.name.empty()) {
+        return;
+    }
 
     // Remove the old function.
-    function_remove(data.name);
+    funcset->remove(data.name);
+
+    // Check if this is a function that we are autoloading.
+    bool is_autoload = funcset->autoloader.autoload_in_progress(data.name);
 
     // Create and store a new function.
     const wchar_t *filename = reader_current_filename();
-
+    auto ins = funcset->funcs.emplace(data.name,
-    const function_map_t::value_type new_pair(
+                                      function_info_t(data, parser.vars(), filename, is_autoload));
-        data.name, function_info_t(data, parser.vars(), filename, is_autoload));
+    assert(ins.second && "Function should not already be present in the table");
-    loaded_functions.insert(new_pair);
+    (void)ins;
 
     // Add event handlers.
     for (const event_description_t &ed : data.events) {
@@ -175,129 +205,111 @@ void function_add(const function_data_t &data, const parser_t &parser) {
 
 std::shared_ptr<const function_properties_t> function_get_properties(const wcstring &name) {
     if (parser_keywords_is_reserved(name)) return nullptr;
-    scoped_rlock locker(functions_lock);
+    auto funcset = function_set.acquire();
-    auto where = loaded_functions.find(name);
+    if (const auto *info = funcset->get_info(name)) {
-    if (where != loaded_functions.end()) {
+        return info->props;
-        return where->second.props;
     }
     return nullptr;
 }
 
 int function_exists(const wcstring &cmd) {
+    ASSERT_IS_MAIN_THREAD();
     if (parser_keywords_is_reserved(cmd)) return 0;
-    scoped_rlock locker(functions_lock);
+    try_autoload(cmd);
-    load(cmd);
+    auto funcset = function_set.acquire();
-    return loaded_functions.find(cmd) != loaded_functions.end();
+    return funcset->funcs.find(cmd) != funcset->funcs.end();
 }
 
 void function_load(const wcstring &cmd) {
+    ASSERT_IS_MAIN_THREAD();
     if (!parser_keywords_is_reserved(cmd)) {
-        scoped_rlock locker(functions_lock);
+        try_autoload(cmd);
-        load(cmd);
     }
 }
 
 int function_exists_no_autoload(const wcstring &cmd, const environment_t &vars) {
     if (parser_keywords_is_reserved(cmd)) return 0;
-    scoped_rlock locker(functions_lock);
+    auto funcset = function_set.acquire();
-    return loaded_functions.find(cmd) != loaded_functions.end() ||
+
-           function_autoloader.can_load(cmd, vars);
+    // Check if we either have the function, or it could be autoloaded.
+    return funcset->get_info(cmd) || funcset->autoloader.can_autoload(cmd);
 }
 
-static bool function_remove_ignore_autoload(const wcstring &name) {
+bool function_set_t::remove(const wcstring &name) {
-    // Note: the lock may be held at this point, but is recursive.
+    size_t amt = funcs.erase(name);
-    scoped_rlock locker(functions_lock);
+    if (amt > 0) {
-
+        event_remove_function_handlers(name);
-    function_map_t::iterator iter = loaded_functions.find(name);
+    }
-
+    return amt > 0;
-    // Not found.  Not erasing.
-    if (iter == loaded_functions.end()) return false;
-
-    // If we are removing an auto-loaded function, prevent it from being auto-reloaded.
-    if (iter->second.is_autoload) function_tombstones.insert(name);
-
-    loaded_functions.erase(iter);
-    event_remove_function_handlers(name);
-    return true;
 }
 
 void function_remove(const wcstring &name) {
-    if (function_remove_ignore_autoload(name)) function_autoloader.unload(name);
+    auto funcset = function_set.acquire();
-}
+    if (funcset->remove(name)) {
-
+        // Prevent re-autoloading this function.
-/// Returns a function by name if it has been loaded, returns false otherwise. Does not autoload.
+        funcset->autoload_tombstones.insert(name);
-static const function_info_t *function_get(const wcstring &name) {
-    // The caller must lock the functions_lock before calling this; however our mutex is currently
-    // recursive, so trylock will never fail. We need a way to correctly check if a lock is locked
-    // (or better yet, make our lock non-recursive).
-    // ASSERT_IS_LOCKED(functions_lock);
-    function_map_t::iterator iter = loaded_functions.find(name);
-    if (iter == loaded_functions.end()) {
-        return NULL;
     }
-    return &iter->second;
 }
 
 bool function_get_definition(const wcstring &name, wcstring &out_definition) {
-    scoped_rlock locker(functions_lock);
+    const auto funcset = function_set.acquire();
-    const function_info_t *func = function_get(name);
+    if (const function_info_t *func = funcset->get_info(name)) {
-    if (func) {
         auto props = func->props;
         if (props && props->parsed_source) {
             out_definition = props->body_node.get_source(props->parsed_source->src);
         }
+        return true;
     }
-    return func != NULL;
+    return false;
 }
 
 std::map<wcstring, env_var_t> function_get_inherit_vars(const wcstring &name) {
-    scoped_rlock locker(functions_lock);
+    const auto funcset = function_set.acquire();
-    const function_info_t *func = function_get(name);
+    const function_info_t *func = funcset->get_info(name);
     return func ? func->inherit_vars : std::map<wcstring, env_var_t>();
 }
 
 bool function_get_desc(const wcstring &name, wcstring &out_desc) {
-    // Empty length string goes to NULL.
+    const auto funcset = function_set.acquire();
-    scoped_rlock locker(functions_lock);
+    const function_info_t *func = funcset->get_info(name);
-    const function_info_t *func = function_get(name);
     if (func && !func->description.empty()) {
         out_desc = _(func->description.c_str());
         return true;
     }
-
     return false;
 }
 
 void function_set_desc(const wcstring &name, const wcstring &desc) {
-    load(name);
+    ASSERT_IS_MAIN_THREAD();
-    scoped_rlock locker(functions_lock);
+    try_autoload(name);
-    function_map_t::iterator iter = loaded_functions.find(name);
+    auto funcset = function_set.acquire();
-    if (iter != loaded_functions.end()) {
+    auto iter = funcset->funcs.find(name);
+    if (iter != funcset->funcs.end()) {
         iter->second.description = desc;
     }
 }
 
 bool function_copy(const wcstring &name, const wcstring &new_name) {
-    bool result = false;
+    auto funcset = function_set.acquire();
-    scoped_rlock locker(functions_lock);
+    auto iter = funcset->funcs.find(name);
-    function_map_t::const_iterator iter = loaded_functions.find(name);
+    if (iter == funcset->funcs.end()) {
-    if (iter != loaded_functions.end()) {
+        // No such function.
-        // This new instance of the function shouldn't be tied to the definition file of the
+        return false;
-        // original, so pass NULL filename, etc.
-        const function_map_t::value_type new_pair(new_name,
-                                                  function_info_t(iter->second, NULL, false));
-        loaded_functions.insert(new_pair);
-        result = true;
     }
-    return result;
+
+    // This new instance of the function shouldn't be tied to the definition file of the
+    // original, so pass NULL filename, etc.
+    // Note this will NOT overwrite an existing function with the new name.
+    // TODO: rationalize if this behavior is desired.
+    funcset->funcs.emplace(new_name, function_info_t(iter->second, nullptr, false));
+    return true;
 }
 
 wcstring_list_t function_get_names(int get_hidden) {
     std::unordered_set<wcstring> names;
-    scoped_rlock locker(functions_lock);
+    auto funcset = function_set.acquire();
     autoload_names(names, get_hidden);
-
+    for (const auto &func : funcset->funcs) {
-    for (const auto &func : loaded_functions) {
         const wcstring &name = func.first;
 
         // Maybe skip hidden.
@@ -310,20 +322,20 @@ wcstring_list_t function_get_names(int get_hidden) {
 }
 
 const wchar_t *function_get_definition_file(const wcstring &name) {
-    scoped_rlock locker(functions_lock);
+    const auto funcset = function_set.acquire();
-    const function_info_t *func = function_get(name);
+    const function_info_t *func = funcset->get_info(name);
     return func ? func->definition_file : NULL;
 }
 
 bool function_is_autoloaded(const wcstring &name) {
-    scoped_rlock locker(functions_lock);
+    const auto funcset = function_set.acquire();
-    const function_info_t *func = function_get(name);
+    const function_info_t *func = funcset->get_info(name);
-    return func->is_autoload;
+    return func ? func->is_autoload : false;
 }
 
 int function_get_definition_lineno(const wcstring &name) {
-    scoped_rlock locker(functions_lock);
+    const auto funcset = function_set.acquire();
-    const function_info_t *func = function_get(name);
+    const function_info_t *func = funcset->get_info(name);
     if (!func) return -1;
     // return one plus the number of newlines at offsets less than the start of our function's
     // statement (which includes the header).
@@ -338,7 +350,22 @@ int function_get_definition_lineno(const wcstring &name) {
     return 1 + std::count(source.begin(), source.begin() + func_start, L'\n');
 }
 
-void function_invalidate_path() { function_autoloader.invalidate(); }
+void function_invalidate_path() {
+    // Remove all autoloaded functions and update the autoload path.
+    // Note we don't want to risk removal during iteration; we expect this to be called
+    // infrequently.
+    auto funcset = function_set.acquire();
+    wcstring_list_t autoloadees;
+    for (const auto &kv : funcset->funcs) {
+        if (kv.second.is_autoload) {
+            autoloadees.push_back(kv.first);
+        }
+    }
+    for (const wcstring &name : autoloadees) {
+        funcset->remove(name);
+    }
+    funcset->autoloader.clear();
+}
 
 // Setup the environment for the function. There are three components of the environment:
 // 1. argv