[libc] Resolve race condition in sub-process test runner.

[PaulkaToast]

Summary

There seems to be a race condition between the pipe closing and the child process death. Likely these two events are not atomic on some versions of linux.

With the removal of WNOHANG we eliminate the race condition, however if the child closes the pipe intentionally then it could result in the test runner hanging. I find this situation less likely, where as I experience failures locally with this race condition rather consistently.

[sivachandra]

libc/utils/testutils/ExecuteFunctionUnix.cpp

73

At this point, this assert is probably pointless.

[abrachet]

LGTM. Sorry about that, I never caught this failing so thanks for catching it!

however if the child closes the pipe intentionally then it could result in the test runner hanging. I find this situation less likely, where as I experience failures locally with this race condition rather consistently.

Agreed I don't think the death test would run ::close(4), and this is a more controlled environment so I think it is fine. If we want we can do the more traditional pattern of setting an alarm and sigwaiting for either SIGALRM or SIGCHLD, but I find signals very hacky.

[sivachandra]

libc/utils/testutils/ExecuteFunctionUnix.cpp

70–75

BTW, it is worth adding a comment here that Pid will not be recycled until the call to waitpid and so waiting on it is safe even after the process actually exits. Every time I read code like this, I end up having to Google for Pid recycling rules.

[PaulkaToast]

Add comment explaining why we wait on pid.

libc/utils/testutils/ExecuteFunctionUnix.cpp

if (::poll(&pollFD, 1, timeoutMS) == -1)
     return ProcessStatus::Error("poll(2) failed");
   // If the pipe wasn't closed by the child yet then timeout has expired.
   if (!(pollFD.revents & POLLHUP)) {
     ::kill(Pid, SIGKILL);
     return ProcessStatus::TimedOut();
   }
 
   int WStatus = 0;
-  int status = ::waitpid(Pid, &WStatus, WNOHANG);
-  assert(status == Pid && "wait call should not block here");
+  // Wait on the pid of the subprocess here so it gets collected by the system
+  // and doesn't turn into a zombie.
+  pid_t status = ::waitpid(Pid, &WStatus, 0);
+  if (status == -1)
+    return ProcessStatus::Error("waitpid(2) failed");
+  assert(status == Pid);
   (void)status;
   return {WStatus};
 }
 
 const char *signalAsString(int Signum) { return ::strsignal(Signum); }
 
 } // namespace testutils
 } // namespace __llvm_libc