Don't use posix_spawn for commands that need to be put into foreground to avoid a race

Fix for race where a command's output may not be fully drained

exec.cpp

@@ -506,11 +506,22 @@ static void do_builtin_io( const char *out, const char *err )
 
 /* Returns whether we can use posix spawn for a given process in a given job.
  Per https://github.com/fish-shell/fish-shell/issues/364 , error handling for file redirections is too difficult with posix_spawn
- So in that case we use fork/exec.
-
+ So in that case we use fork/exec
+ 
+ Furthermore, to avoid the race between the caller calling tcsetpgrp() and the client checking the foreground process group, we don't use posix_spawn if we're going to foreground the process. (If we use fork(), we can call tcsetpgrp after the fork, before the exec, and avoid the racse).
 */
 static bool can_use_posix_spawn_for_job(const job_t *job, const process_t *process)
 {
+    if ( job_get_flag( job, JOB_CONTROL ) )
+    {
+        /* We are going to use job control; therefore when we launch this job it will get its own process group ID. But will it be foregrounded? */
+        if ( job_get_flag( job, JOB_TERMINAL ) && job_get_flag( job, JOB_FOREGROUND ) )
+        {
+            /* It will be foregrounded, so we will call tcsetpgrp(), therefore do not use posix_spawn */
+            return false;
+        }
+    }
+
     bool result = true;
     for (size_t idx = 0; idx < job->io.size(); idx++)
     {
@@ -1142,7 +1153,7 @@ void exec( parser_t &parser, job_t *j )
 					( ! get_stdout_buffer().empty() ) && 
 					( buffer_stdout ) )
 				{
-					std::string res = wcs2string( get_stdout_buffer() );
+					const std::string res = wcs2string( get_stdout_buffer() );
 					io->out_buffer_append( res.c_str(), res.size() );
 					skip_fork = 1;
 				}
@@ -1268,6 +1279,10 @@ void exec( parser_t &parser, job_t *j )
                     {
                         /* We successfully made the attributes and actions; actually call posix_spawn */
                         int spawn_ret = posix_spawn(&pid, actual_cmd, &actions, &attr, argv, envv);
+                        
+                        /* This usleep can be used to test for various race conditions (https://github.com/fish-shell/fish-shell/issues/360) */
+                        //usleep(10000);
+                        
                         if (spawn_ret != 0)
                         {
                             safe_report_exec_error(spawn_ret, actual_cmd, argv, envv);

postfork.cpp

@@ -386,6 +386,7 @@ bool fork_actions_make_spawn_properties(posix_spawnattr_t *attr, posix_spawn_fil
         // means that the process becomes its own
         // group leader, which is what set_child_group did
         // in this case. So we want this to be 0 if j->pgid is 0.
+        //if (!j->pgid) j->pgid = getpid();
         desired_parent_group_id = j->pgid;
     }
     

proc.cpp

@@ -1110,14 +1110,6 @@ void job_continue (job_t *j, int cont)
 				}
 				while (got_signal && !quit);
                 
-                if (quit) {
-                    // It's possible that the job will produce output and exit before we've even read from it.
-                    // We'll eventually read the output, but it may be after we've executed subsequent calls
-                    // This is why my prompt colors kept getting screwed up - the builtin echo calls
-                    // were sometimes having their output combined with the set_color calls in the wrong order!
-                    read_try(j);
-                }
-
 				if( !quit )
 				{
 					
@@ -1175,6 +1167,14 @@ void job_continue (job_t *j, int cont)
 		
 		if( job_is_completed( j ))
 		{
+        
+            // It's possible that the job will produce output and exit before we've even read from it.
+            // We'll eventually read the output, but it may be after we've executed subsequent calls
+            // This is why my prompt colors kept getting screwed up - the builtin echo calls
+            // were sometimes having their output combined with the set_color calls in the wrong order!
+            read_try(j);
+
+        
 			process_t *p = j->first_process;
 			while( p->next )
 				p = p->next;

reader.cpp

@@ -644,8 +644,6 @@ void reader_write_title()
 */
 static void exec_prompt()
 {
-	size_t i;
-
     wcstring_list_t prompt_list;
 	
 	if( data->prompt.size() )
@@ -664,7 +662,7 @@ static void exec_prompt()
 	
     data->prompt_buff.clear();
 	
-	for( i = 0; i < prompt_list.size(); i++ )
+	for( size_t i = 0; i < prompt_list.size(); i++ )
 	{
         if (i > 0) data->prompt_buff += L'\n';
         data->prompt_buff += prompt_list.at(i);