fix regression how fish_escape_delay_ms is handled

Fish 2.6.0 introduced a regression that keeps setting
`fish_escape_delay_ms` as a uvar from working. This also fixes a related
problem: callbacks generated from the initial loading of universal vars
were not being acted on.

Fixes #4196

CHANGELOG.md

@@ -26,6 +26,7 @@
 - Fix setting `$COLUMNS` and `$LINES` before first prompt is displayed (#4141).
 - `read` failures due to too much data should define the var (#4180).
 - multiple `read` commands in non-interactive scripts were broken in fish 2.6.0 (#4206).
+- Fix regression involving universal variables with side-effects at startup such as `set -U fish_escape_delay_ms 10` (#4196).
 - Added completions for:
  - `as` (#4130).
  - `jest` (#4142).

src/env.cpp

@@ -588,7 +588,8 @@ static bool variable_is_colon_delimited_var(const wcstring &str) {
 /// React to modifying the given variable.
 static void react_to_variable_change(const wcstring &key) {
     // Don't do any of this until `env_init()` has run. We only want to do this in response to
-    // variables set by the user; e.g., in a script like *config.fish* or interactively.
+    // variables set by the user; e.g., in a script like *config.fish* or interactively or as part
+    // of loading the universal variables for the first time.
     if (!env_initialized) return;
 
     if (var_is_locale(key)) {
@@ -753,6 +754,26 @@ void misc_init() {
 #endif  // OS_IS_MS_WINDOWS
 }
 
+static void env_universal_callbacks(callback_data_list_t &callbacks) {
+    for (size_t i = 0; i < callbacks.size(); i++) {
+        const callback_data_t &data = callbacks.at(i);
+        universal_callback(data.type, data.key.c_str());
+    }
+}
+
+void env_universal_barrier() {
+    ASSERT_IS_MAIN_THREAD();
+    if (!uvars()) return;
+
+    callback_data_list_t callbacks;
+    bool changed = uvars()->sync(callbacks);
+    if (changed) {
+        universal_notifier_t::default_notifier().post_notification();
+    }
+
+    env_universal_callbacks(callbacks);
+}
+
 void env_init(const struct config_paths_t *paths /* or NULL */) {
     // These variables can not be altered directly by the user.
     const wchar_t *const ro_keys[] = {
@@ -869,11 +890,6 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
     env_set_termsize();    // initialize the terminal size variables
     env_set_read_limit();  // initialize the read_byte_limit
 
-    // Set up universal variables. The empty string means to use the deafult path.
-    assert(s_universal_variables == NULL);
-    s_universal_variables = new env_universal_t(L"");
-    s_universal_variables->load();
-
     // Set g_use_posix_spawn. Default to true.
     env_var_t use_posix_spawn = env_get_string(L"fish_use_posix_spawn");
     g_use_posix_spawn =
@@ -882,11 +898,23 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
     // Set fish_bind_mode to "default".
     env_set(FISH_BIND_MODE_VAR, DEFAULT_BIND_MODE, ENV_GLOBAL);
 
+    // This is somewhat subtle. At this point we consider our environment to be sufficiently
+    // initialized that we can react to changes to variables. Prior to doing this we expect that the
+    // code for setting vars that might have side-effects will do whatever
+    // `react_to_variable_change()` would do for that var.
+    env_initialized = true;
+
+    // Set up universal variables. The empty string means to use the default path.
+    assert(s_universal_variables == NULL);
+    s_universal_variables = new env_universal_t(L"");
+    callback_data_list_t callbacks;
+    s_universal_variables->load(callbacks);
+    env_universal_callbacks(callbacks);
+
     // Now that the global scope is fully initialized, add a toplevel local scope. This same local
     // scope will persist throughout the lifetime of the fish process, and it will ensure that `set
     // -l` commands run at the command-line don't affect the global scope.
     env_push(false);
-    env_initialized = true;
 }
 
 /// Search all visible scopes in order for the specified key. Return the first scope in which it was
@@ -1482,23 +1510,6 @@ env_vars_snapshot_t::env_vars_snapshot_t(const wchar_t *const *keys) {
     }
 }
 
-void env_universal_barrier() {
-    ASSERT_IS_MAIN_THREAD();
-    if (uvars()) {
-        callback_data_list_t changes;
-        bool changed = uvars()->sync(&changes);
-        if (changed) {
-            universal_notifier_t::default_notifier().post_notification();
-        }
-
-        // Post callbacks.
-        for (size_t i = 0; i < changes.size(); i++) {
-            const callback_data_t &data = changes.at(i);
-            universal_callback(data.type, data.key.c_str());
-        }
-    }
-}
-
 env_vars_snapshot_t::env_vars_snapshot_t() {}
 
 // The "current" variables are not a snapshot at all, but instead trampoline to env_get_string, etc.

src/env_universal_common.cpp

@@ -346,9 +346,7 @@ wcstring_list_t env_universal_t::get_names(bool show_exported, bool show_unexpor
 // Given a variable table, generate callbacks representing the difference between our vars and the
 // new vars.
 void env_universal_t::generate_callbacks(const var_table_t &new_vars,
-                                         callback_data_list_t *callbacks) const {
-    assert(callbacks != NULL);
-
+                                         callback_data_list_t &callbacks) const {
     // Construct callbacks for erased values.
     for (var_table_t::const_iterator iter = this->vars.begin(); iter != this->vars.end(); ++iter) {
         const wcstring &key = iter->first;
@@ -360,7 +358,7 @@ void env_universal_t::generate_callbacks(const var_table_t &new_vars,
 
         // If the value is not present in new_vars, it has been erased.
         if (new_vars.find(key) == new_vars.end()) {
-            callbacks->push_back(callback_data_t(ERASE, key, L""));
+            callbacks.push_back(callback_data_t(ERASE, key, L""));
         }
     }
 
@@ -379,7 +377,7 @@ void env_universal_t::generate_callbacks(const var_table_t &new_vars,
         if (existing == this->vars.end() || existing->second.exportv != new_entry.exportv ||
             existing->second.val != new_entry.val) {
             // Value has changed.
-            callbacks->push_back(
+            callbacks.push_back(
                 callback_data_t(new_entry.exportv ? SET_EXPORT : SET, key, new_entry.val));
         }
     }
@@ -408,7 +406,7 @@ void env_universal_t::acquire_variables(var_table_t *vars_to_acquire) {
     this->vars = std::move(*vars_to_acquire);
 }
 
-void env_universal_t::load_from_fd(int fd, callback_data_list_t *callbacks) {
+void env_universal_t::load_from_fd(int fd, callback_data_list_t &callbacks) {
     ASSERT_IS_LOCKED(lock);
     assert(fd >= 0);
     // Get the dev / inode.
@@ -420,9 +418,7 @@ void env_universal_t::load_from_fd(int fd, callback_data_list_t *callbacks) {
         var_table_t new_vars = this->read_message_internal(fd);
 
         // Announce changes.
-        if (callbacks != NULL) {
         this->generate_callbacks(new_vars, callbacks);
-        }
 
         // Acquire the new variables.
         this->acquire_variables(&new_vars);
@@ -430,7 +426,7 @@ void env_universal_t::load_from_fd(int fd, callback_data_list_t *callbacks) {
     }
 }
 
-bool env_universal_t::load_from_path(const wcstring &path, callback_data_list_t *callbacks) {
+bool env_universal_t::load_from_path(const wcstring &path, callback_data_list_t &callbacks) {
     ASSERT_IS_LOCKED(lock);
 
     // Check to see if the file is unchanged. We do this again in load_from_fd, but this avoids
@@ -507,12 +503,11 @@ bool env_universal_t::move_new_vars_file_into_place(const wcstring &src, const w
     return ret == 0;
 }
 
-bool env_universal_t::load() {
-    callback_data_list_t callbacks;
+bool env_universal_t::load(callback_data_list_t &callbacks) {
     const wcstring vars_path =
         explicit_vars_path.empty() ? default_vars_path() : explicit_vars_path;
     scoped_lock locker(lock);
-    bool success = load_from_path(vars_path, &callbacks);
+    bool success = load_from_path(vars_path, callbacks);
     if (!success && !tried_renaming && errno == ENOENT) {
         // We failed to load, because the file was not found. Older fish used the hostname only. Try
         // moving the filename based on the hostname into place; if that succeeds try again.
@@ -523,10 +518,11 @@ bool env_universal_t::load() {
             const wcstring hostname_path = wdirname(vars_path) + L'/' + hostname_id;
             if (0 == wrename(hostname_path, vars_path)) {
                 // We renamed - try again.
-                success = this->load();
+                success = this->load(callbacks);
             }
         }
     }
+
     return success;
 }
 
@@ -645,7 +641,7 @@ bool env_universal_t::open_and_acquire_lock(const wcstring &path, int *out_fd) {
 
 // Returns true if modified variables were written, false if not. (There may still be variable
 // changes due to other processes on a false return).
-bool env_universal_t::sync(callback_data_list_t *callbacks) {
+bool env_universal_t::sync(callback_data_list_t &callbacks) {
     debug(5, L"universal log sync");
     scoped_lock locker(lock);
     // Our saving strategy:

src/env_universal_common.h

@@ -41,8 +41,8 @@ class env_universal_t {
 
     mutable pthread_mutex_t lock;
     bool tried_renaming;
-    bool load_from_path(const wcstring &path, callback_data_list_t *callbacks);
-    void load_from_fd(int fd, callback_data_list_t *callbacks);
+    bool load_from_path(const wcstring &path, callback_data_list_t &callbacks);
+    void load_from_fd(int fd, callback_data_list_t &callbacks);
 
     void set_internal(const wcstring &key, const wcstring &val, bool exportv, bool overwrite);
     bool remove_internal(const wcstring &name);
@@ -58,7 +58,7 @@ class env_universal_t {
 
     // Given a variable table, generate callbacks representing the difference between our vars and
     // the new vars.
-    void generate_callbacks(const var_table_t &new_vars, callback_data_list_t *callbacks) const;
+    void generate_callbacks(const var_table_t &new_vars, callback_data_list_t &callbacks) const;
 
     // Given a variable table, copy unmodified values into self. May destructively modified
     // vars_to_acquire.
@@ -87,11 +87,11 @@ class env_universal_t {
     wcstring_list_t get_names(bool show_exported, bool show_unexported) const;
 
     /// Loads variables at the correct path.
-    bool load();
+    bool load(callback_data_list_t &callbacks);
 
     /// Reads and writes variables at the correct path. Returns true if modified variables were
     /// written.
-    bool sync(callback_data_list_t *callbacks);
+    bool sync(callback_data_list_t &callbacks);
 };
 
 /// The "universal notifier" is an object responsible for broadcasting and receiving universal

src/fish_tests.cpp

@@ -2621,12 +2621,13 @@ static void test_input() {
 #define UVARS_TEST_PATH L"test/fish_uvars_test/varsfile.txt"
 
 static int test_universal_helper(int x) {
+    callback_data_list_t callbacks;
     env_universal_t uvars(UVARS_TEST_PATH);
     for (int j = 0; j < UVARS_PER_THREAD; j++) {
         const wcstring key = format_string(L"key_%d_%d", x, j);
         const wcstring val = format_string(L"val_%d_%d", x, j);
         uvars.set(key, val, false);
-        bool synced = uvars.sync(NULL);
+        bool synced = uvars.sync(callbacks);
         if (!synced) {
             err(L"Failed to sync universal variables after modification");
         }
@@ -2634,7 +2635,7 @@ static int test_universal_helper(int x) {
 
     // Last step is to delete the first key.
     uvars.remove(format_string(L"key_%d_%d", x, 0));
-    bool synced = uvars.sync(NULL);
+    bool synced = uvars.sync(callbacks);
     if (!synced) {
         err(L"Failed to sync universal variables after deletion");
     }
@@ -2652,7 +2653,8 @@ static void test_universal() {
     iothread_drain_all();
 
     env_universal_t uvars(UVARS_TEST_PATH);
-    bool loaded = uvars.load();
+    callback_data_list_t callbacks;
+    bool loaded = uvars.load(callbacks);
     if (!loaded) {
         err(L"Failed to load universal variables");
     }
@@ -2685,6 +2687,8 @@ static bool callback_data_less_than(const callback_data_t &a, const callback_dat
 
 static void test_universal_callbacks() {
     say(L"Testing universal callbacks");
+    callback_data_list_t callbacks;
+
     if (system("mkdir -p test/fish_uvars_test/")) err(L"mkdir failed");
     env_universal_t uvars1(UVARS_TEST_PATH);
     env_universal_t uvars2(UVARS_TEST_PATH);
@@ -2698,23 +2702,23 @@ static void test_universal_callbacks() {
     uvars1.set(L"kappa", L"1", false);
     uvars1.set(L"omicron", L"1", false);
 
-    uvars1.sync(NULL);
-    uvars2.sync(NULL);
+    uvars1.sync(callbacks);
+    uvars2.sync(callbacks);
 
     // Change uvars1.
     uvars1.set(L"alpha", L"2", false);    // changes value
     uvars1.set(L"beta", L"1", true);      // changes export
     uvars1.remove(L"delta");              // erases value
     uvars1.set(L"epsilon", L"1", false);  // changes nothing
-    uvars1.sync(NULL);
+    uvars1.sync(callbacks);
 
     // Change uvars2. It should treat its value as correct and ignore changes from uvars1.
     uvars2.set(L"lambda", L"1", false);  // same value
     uvars2.set(L"kappa", L"2", false);   // different value
 
     // Now see what uvars2 sees.
-    callback_data_list_t callbacks;
-    uvars2.sync(&callbacks);
+    callbacks.clear();
+    uvars2.sync(callbacks);
 
     // Sort them to get them in a predictable order.
     std::sort(callbacks.begin(), callbacks.end(), callback_data_less_than);

src/input_common.cpp

@@ -60,7 +60,6 @@ static int (*interrupt_handler)();
 
 void input_common_init(int (*ih)()) {
     interrupt_handler = ih;
-    update_wait_on_escape_ms();
 }
 
 void input_common_destroy() {}