Remove get_proc_had_barrier

Prior to this change, fish used a global flag to decide if we should check
for changes to universal variables. This flag was then checked at arbitrary
locations, potentially triggering variable updates and event handlers for
those updates; this was very hard to reason about.

Switch to triggering a universal variable update at a fixed location,
after running an external command.  The common case is that the variable
file has not changed, which we can identify with just a stat() call, so
this is pretty cheap.

src/env.cpp

@@ -658,10 +658,6 @@ int env_stack_t::set_internal(const wcstring &key, env_mode_flags_t input_var_mo
                 }
             }
         } else {
-            if (!get_proc_had_barrier()) {
-                set_proc_had_barrier(true);
-                env_universal_barrier();
-            }
             if (uvars() && uvars()->get(key)) {
                 // Modifying an existing universal variable.
                 env_set_internal_universal(key, std::move(val), var_mode, this);
@@ -907,13 +903,6 @@ maybe_t<env_var_t> env_stack_t::get(const wcstring &key, env_mode_flags_t mode)
 
     if (!search_universal) return none();
 
-    // Another hack. Only do a universal barrier on the main thread (since it can change variable
-    // values). Make sure we do this outside the env_lock because it may itself call `get()`.
-    if (is_main_thread() && !get_proc_had_barrier()) {
-        set_proc_had_barrier(true);
-        env_universal_barrier();
-    }
-
     // Okay, we couldn't find a local or global var given the requirements. If there is a matching
     // universal var return that.
     if (uvars()) {
@@ -926,7 +915,7 @@ maybe_t<env_var_t> env_stack_t::get(const wcstring &key, env_mode_flags_t mode)
     return none();
 }
 
-void env_universal_barrier() { env_stack_t::principal().universal_barrier(); }
+void env_universal_barrier() {env_stack_t::principal().universal_barrier(); }
 
 /// Returns true if the specified scope or any non-shadowed non-global subscopes contain an exported
 /// variable.

src/exec.cpp

@@ -926,20 +926,6 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr<
         }
     }
 
-    // This call is used so the global environment variable array is regenerated, if needed,
-    // before the fork. That way, we avoid a lot of duplicate work where EVERY child would need
-    // to generate it, since that result would not get written back to the parent. This call
-    // could be safely removed, but it would result in slightly lower performance - at least on
-    // uniprocessor systems.
-    if (p->type == process_type_t::external) {
-        // Apply universal barrier so we have the most recent uvar changes
-        if (!get_proc_had_barrier()) {
-            set_proc_had_barrier(true);
-            env_universal_barrier();
-        }
-        parser.vars().export_arr();
-    }
-
     // Execute the process.
     p->check_generations_before_launch();
     switch (p->type) {

src/parse_execution.cpp

@@ -1274,9 +1274,10 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> jo
             remove_job(job.get());
         }
 
-        // Only external commands require a new fishd barrier.
+        // Update universal vaiables on external conmmands.
+        // TODO: justify this, why not on every command?
         if (job_contained_external_command) {
-            set_proc_had_barrier(false);
+            parser->vars().universal_barrier();
         }
     }
 

src/proc.cpp

@@ -75,8 +75,6 @@ int no_exec = 0;
 
 static int is_interactive = -1;
 
-static bool proc_had_barrier = false;
-
 bool shell_is_interactive() {
     ASSERT_IS_MAIN_THREAD();
     // is_interactive is statically initialized to -1. Ensure it has been dynamically set
@@ -85,16 +83,6 @@ bool shell_is_interactive() {
     return is_interactive > 0;
 }
 
-bool get_proc_had_barrier() {
-    ASSERT_IS_MAIN_THREAD();
-    return proc_had_barrier;
-}
-
-void set_proc_had_barrier(bool flag) {
-    ASSERT_IS_MAIN_THREAD();
-    proc_had_barrier = flag;
-}
-
 /// A stack containing the values of is_interactive. Used by proc_push_interactive and
 /// proc_pop_interactive.
 static std::vector<int> interactive_stack;

src/proc.h

@@ -467,15 +467,6 @@ bool job_list_is_empty(void);
 /// A helper function to more easily access the job list
 job_list_t &jobs();
 
-/// Whether a universal variable barrier roundtrip has already been made for the currently executing
-/// command. Such a roundtrip only needs to be done once on a given command, unless a universal
-/// variable value is changed. Once this has been done, this variable is set to 1, so that no more
-/// roundtrips need to be done.
-///
-/// Both setting it to one when it should be zero and the opposite may cause concurrency bugs.
-bool get_proc_had_barrier();
-void set_proc_had_barrier(bool flag);
-
 /// The current job control mode.
 ///
 /// Must be one of job_control_t::all, job_control_t::interactive and job_control_t::none.