Try to rationalize universal variable syncing

Prior to this commit, setting a universal variable may trigger syncing
against the file which will modify other universal variables. But if we
want to support multiple environments we need the parser to decide when to
sync uvars. Shift the decision of when to sync to the parser itself. When a
universal variable is modified, now we just set a flag and it's up to the
(main) parser when to pick it up. This is hopefully just a refactoring with
no user-visible changes.

src/env.cpp

@@ -65,11 +65,12 @@ static acquired_lock<env_universal_t> uvars() {
     return s_universal_variables->acquire();
 }
 
+/// Set when a universal variable has been modified but not yet been written to disk via sync().
+static relaxed_atomic_bool_t s_uvars_locally_modified{false};
+
 /// Whether we were launched with no_config; in this case setting a uvar instead sets a global.
 static relaxed_atomic_bool_t s_uvar_scope_is_global{false};
 
-bool env_universal_barrier() { return env_stack_t::principal().universal_barrier(); }
-
 namespace {
 struct electric_var_t {
     enum {
@@ -430,7 +431,9 @@ void env_init(const struct config_paths_t *paths, bool do_uvars, bool default_pa
         // Set up universal variables using the default path.
         callback_data_list_t callbacks;
         uvars()->initialize(callbacks);
-        env_universal_callbacks(&vars, callbacks);
+        for (const callback_data_t &cb : callbacks) {
+            env_dispatch_var_change(cb.key, vars);
+        }
 
         // Do not import variables that have the same name and value as
         // an exported universal variable. See issues #5258 and #5348.
@@ -1289,23 +1292,25 @@ mod_result_t env_stack_impl_t::remove(const wcstring &key, int mode) {
     return result;
 }
 
-bool env_stack_t::universal_barrier() {
-    // Only perform universal barriers for the principal env stack.
-    // This means that changes from other fish processes will only be visible when the "main thread
-    // runs."
-    if (!is_principal()) return false;
-
-    ASSERT_IS_MAIN_THREAD();
-    if (s_uvar_scope_is_global) return false;
+std::vector<event_t> env_stack_t::universal_sync(bool always) {
+    if (s_uvar_scope_is_global) return {};
+    if (!always && !s_uvars_locally_modified) return {};
+    s_uvars_locally_modified = false;
 
     callback_data_list_t callbacks;
     bool changed = uvars()->sync(callbacks);
     if (changed) {
         universal_notifier_t::default_notifier().post_notification();
     }
-
-    env_universal_callbacks(this, callbacks);
-    return changed || !callbacks.empty();
+    // React internally to changes to special variables like LANG, and populate on-variable events.
+    std::vector<event_t> result;
+    for (const callback_data_t &cb : callbacks) {
+        env_dispatch_var_change(cb.key, *this);
+        event_t evt =
+            cb.is_erase() ? event_t::variable_erase(cb.key) : event_t::variable_set(cb.key);
+        result.push_back(std::move(evt));
+    }
+    return result;
 }
 
 statuses_t env_stack_t::get_last_statuses() const {
@@ -1368,9 +1373,9 @@ int env_stack_t::set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t
             env_dispatch_var_change(key, *this);
         }
     }
-    // If the principal stack modified universal variables, then post a barrier.
-    if (ret.uvar_modified && is_principal()) {
-        universal_barrier();
+    // Mark if we modified a uvar.
+    if (ret.uvar_modified) {
+        s_uvars_locally_modified = true;
     }
     return ret.status;
 }
@@ -1393,8 +1398,8 @@ int env_stack_t::remove(const wcstring &key, int mode) {
             env_dispatch_var_change(key, *this);
         }
     }
-    if (ret.uvar_modified && is_principal()) {
-        universal_barrier();
+    if (ret.uvar_modified) {
+        s_uvars_locally_modified = true;
     }
     return ret.status;
 }

src/env.h

@@ -259,10 +259,6 @@ class env_stack_t final : public environment_t {
     /// Pop the variable stack. Used for implementing local variables for functions and for-loops.
     void pop();
 
-    /// Synchronizes all universal variable changes: writes everything out, reads stuff in.
-    /// \return true if something changed, false otherwise.
-    bool universal_barrier();
-
     /// Returns an array containing all exported variables in a format suitable for execv.
     std::shared_ptr<owning_null_terminated_array_t> export_arr();
 
@@ -284,6 +280,12 @@ class env_stack_t final : public environment_t {
     /// Slightly optimized implementation.
     wcstring get_pwd_slash() const override;
 
+    /// Synchronizes universal variable changes.
+    /// If \p always is set, perform synchronization even if there's no pending changes from this
+    /// instance (that is, look for changes from other fish instances).
+    /// \return a list of events for changed variables.
+    std::vector<event_t> universal_sync(bool always);
+
     // Compatibility hack; access the "environment stack" from back when there was just one.
     static const std::shared_ptr<env_stack_t> &principal_ref();
     static env_stack_t &principal() { return *principal_ref(); }
@@ -297,10 +299,6 @@ bool get_use_posix_spawn();
 
 extern bool term_has_xn;  // does the terminal have the "eat_newline_glitch"
 
-/// Synchronizes all universal variable changes: writes everything out, reads stuff in.
-/// \return true if any value changed.
-bool env_universal_barrier();
-
 /// Returns true if we think the terminal supports setting its title.
 bool term_supports_setting_title();
 

src/env_dispatch.cpp

@@ -194,19 +194,6 @@ void env_dispatch_var_change(const wcstring &key, env_stack_t &vars) {
     s_var_dispatch_table->dispatch(key, vars);
 }
 
-// Trigger events due to a universal variable changing.
-void env_universal_callbacks(env_stack_t *stack, const callback_data_list_t &callbacks) {
-    for (const callback_data_t &cb : callbacks) {
-        env_dispatch_var_change(cb.key, *stack);
-
-        // TODO: eliminate this principal_parser. Need to rationalize how multiple threads work
-        // here.
-        event_t evt =
-            cb.is_erase() ? event_t::variable_erase(cb.key) : event_t::variable_set(cb.key);
-        event_fire(parser_t::principal_parser(), evt);
-    }
-}
-
 static void handle_fish_term_change(const env_stack_t &vars) {
     update_fish_color_support(vars);
     reader_schedule_prompt_repaint();

src/env_dispatch.h

@@ -9,13 +9,14 @@
 #include "common.h"
 #include "env_universal_common.h"
 
-/// Initialize variable dispatch.
 class environment_t;
+class env_stack_t;
+class parser_t;
+
+/// Initialize variable dispatch.
 void env_dispatch_init(const environment_t &vars);
 
-class env_stack_t;
+/// React to changes in variables like LANG which require running some code.
 void env_dispatch_var_change(const wcstring &key, env_stack_t &vars);
 
-void env_universal_callbacks(env_stack_t *stack, const callback_data_list_t &callbacks);
-
 #endif

src/fish.cpp

@@ -510,6 +510,7 @@ int main(int argc, char **argv) {
     reader_init();
 
     parser_t &parser = parser_t::principal_parser();
+    parser.set_syncs_uvars(!opts.no_config);
 
     if (!opts.no_exec && !opts.no_config) {
         read_init(parser, paths);

src/input.cpp

@@ -351,6 +351,10 @@ void inputter_t::select_interrupted() /* override */ {
     this->push_front(char_event_t{char_event_type_t::check_exit});
 }
 
+void inputter_t::uvar_change_notified() /* override */ {
+    this->parser_->sync_uvars_and_fire(true /* always */);
+}
+
 void inputter_t::function_push_arg(wchar_t arg) { input_function_args_.push_back(arg); }
 
 wchar_t inputter_t::function_pop_arg() {

src/input.h

@@ -60,6 +60,9 @@ class inputter_t final : private input_event_queue_t {
     // Called when select() is interrupted by a signal.
     void select_interrupted() override;
 
+    // Called when we are notified of a uvar change.
+    void uvar_change_notified() override;
+
     void function_push_arg(wchar_t arg);
     void function_push_args(readline_cmd_t code);
     void mapping_execute(const input_mapping_t &m, const command_handler_t &command_handler);

src/input_common.cpp

@@ -180,7 +180,7 @@ char_event_t input_event_queue_t::readch() {
                 break;
 
             case readb_uvar_notified:
-                env_universal_barrier();
+                this->uvar_change_notified();
                 break;
 
             case readb_ioport_notified:
@@ -266,4 +266,5 @@ void input_event_queue_t::promote_interruptions_to_front() {
 
 void input_event_queue_t::prepare_to_select() {}
 void input_event_queue_t::select_interrupted() {}
+void input_event_queue_t::uvar_change_notified() {}
 input_event_queue_t::~input_event_queue_t() = default;

src/input_common.h

@@ -225,6 +225,10 @@ class input_event_queue_t {
     /// Override point for when when select() is interrupted by a signal. The default does nothing.
     virtual void select_interrupted();
 
+    /// Override point for when when select() is interrupted by the universal variable notifier.
+    /// The default does nothing.
+    virtual void uvar_change_notified();
+
     virtual ~input_event_queue_t();
 
    private:

src/parse_execution.cpp

@@ -1383,10 +1383,8 @@ end_execution_reason_t parse_execution_context_t::run_1_job(const ast::job_t &jo
         }
 
         // Update universal variables on external commands.
-        // TODO: justify this, why not on every command?
-        if (job->has_external_proc()) {
-            parser->vars().universal_barrier();
-        }
+        // We only incorporate external changes if we had an external proc, for hysterical raisins.
+        parser->sync_uvars_and_fire(job->has_external_proc() /* always */);
 
         // If the job got a SIGINT or SIGQUIT, then we're going to start unwinding.
         if (!cancel_signal) cancel_signal = job->group->get_cancel_signal();

src/parser.cpp

@@ -72,6 +72,15 @@ int parser_t::set_var_and_fire(const wcstring &key, env_mode_flags_t mode, wcstr
     return set_var_and_fire(key, mode, std::move(vals));
 }
 
+void parser_t::sync_uvars_and_fire(bool always) {
+    if (this->syncs_uvars_) {
+        auto evts = this->vars().universal_sync(always);
+        for (const auto &evt : evts) {
+            event_fire(*this, evt);
+        }
+    }
+}
+
 block_t *parser_t::push_block(block_t &&block) {
     block.src_lineno = parser_t::get_lineno();
     block.src_filename = parser_t::current_filename();

src/parser.h

@@ -259,13 +259,20 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     /// This is in "reverse" order: the topmost block is at the front. This enables iteration from
     /// top down using range-based for loops.
     std::deque<block_t> block_list;
+
     /// The 'depth' of the fish call stack.
     int eval_level = -1;
+
     /// Set of variables for the parser.
     const std::shared_ptr<env_stack_t> variables;
+
     /// Miscellaneous library data.
     library_data_t library_data{};
 
+    /// If set, we synchronize universal variables after external commands,
+    /// including sending on-variable change events.
+    bool syncs_uvars_{false};
+
     /// List of profile items.
     /// This must be a deque because we return pointers to them to callers,
     /// who may hold them across blocks (which would cause reallocations internal
@@ -381,6 +388,11 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     int set_var_and_fire(const wcstring &key, env_mode_flags_t mode, wcstring val);
     int set_var_and_fire(const wcstring &key, env_mode_flags_t mode, wcstring_list_t vals);
 
+    /// Update any universal variables and send event handlers.
+    /// If \p always is set, then do it even if we have no pending changes (that is, look for
+    /// changes from other fish instances); otherwise only sync if this instance has changed uvars.
+    void sync_uvars_and_fire(bool always = false);
+
     /// Pushes a new block. Returns a pointer to the block, stored in the parser. The pointer is
     /// valid until the call to pop_block().
     block_t *push_block(block_t &&b);
@@ -429,6 +441,9 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     /// \return whether the number of functions in the stack exceeds our stack depth limit.
     bool function_stack_is_overflowing() const;
 
+    /// Mark whether we should sync universal variables.
+    void set_syncs_uvars(bool flag) { syncs_uvars_ = flag; }
+
     /// \return a shared pointer reference to this parser.
     std::shared_ptr<parser_t> shared();