From: Alexey Spiridonov <lesha@fb.com>
Date: Mon, 27 Jul 2015 02:32:03 +0000 (-0700)
Subject: Improve waitpid error handling
X-Git-Tag: v0.52.0~2
X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=b10e3e8663d4d7cb8ca7246db980b5f3e5e7bb97

Improve waitpid error handling

Summary: Using `checkUnixError` after `waitpid()` is confusing and useless, because the only two possible errors are `ECHILD` (some other part of the program waited for this process!?) and `EINVAL` (invalid options, which are hardcoded in both `Subprocess::wait()` and `poll()`). Neither of these are recoverable. Moreover, even if the caller catches the exception, `~Subprocess` is still booby-trapped to `abort()` since its status remains `RUNNING`.

In short, just abort.

Reviewed By: @yfeldblum

Differential Revision: D2079415
---

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index 7ecc8de4..b517c55d 100644
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -542,7 +542,10 @@ ProcessReturnCode Subprocess::poll() {
   DCHECK_GT(pid_, 0);
   int status;
   pid_t found = ::waitpid(pid_, &status, WNOHANG);
-  checkUnixError(found, "waitpid");
+  // The spec guarantees that EINTR does not occur with WNOHANG, so the only
+  // two remaining errors are ECHILD (other code reaped the child?), or
+  // EINVAL (cosmic rays?), both of which merit an abort:
+  PCHECK(found != -1) << "waitpid(" << pid_ << ", &status, WNOHANG)";
   if (found != 0) {
     // Though the child process had quit, this call does not close the pipes
     // since its descendants may still be using them.
@@ -568,7 +571,9 @@ ProcessReturnCode Subprocess::wait() {
   do {
     found = ::waitpid(pid_, &status, 0);
   } while (found == -1 && errno == EINTR);
-  checkUnixError(found, "waitpid");
+  // The only two remaining errors are ECHILD (other code reaped the
+  // child?), or EINVAL (cosmic rays?), and both merit an abort:
+  PCHECK(found != -1) << "waitpid(" << pid_ << ", &status, WNOHANG)";
   // Though the child process had quit, this call does not close the pipes
   // since its descendants may still be using them.
   DCHECK_EQ(found, pid_);
diff --git a/folly/Subprocess.h b/folly/Subprocess.h
index b7c377e6..0354b4b2 100644
--- a/folly/Subprocess.h
+++ b/folly/Subprocess.h
@@ -410,11 +410,12 @@ class Subprocess {
   ProcessReturnCode returnCode() const { return returnCode_; }
 
   /**
-   * Poll the child's status and return it, return -1 if the process
-   * is still running.  NOTE that it is illegal to call poll again after
-   * poll indicated that the process has terminated, or to call poll on a
-   * process that hasn't been successfully started (the constructor threw an
-   * exception).
+   * Poll the child's status and return it. Return the exit status if the
+   * subprocess had quit, or RUNNING otherwise.  Throws an std::logic_error
+   * if called on a Subprocess whose status is no longer RUNNING.  No other
+   * exceptions are possible.  Aborts on egregious violations of contract,
+   * e.g. if you wait for the underlying process without going through this
+   * Subprocess instance.
    */
   ProcessReturnCode poll();
 
@@ -426,9 +427,10 @@ class Subprocess {
   bool pollChecked();
 
   /**
-   * Wait for the process to terminate and return its status.
-   * Similarly to poll, it is illegal to call wait after the process
-   * has already been reaped or if the process has not successfully started.
+   * Wait for the process to terminate and return its status.  Like poll(),
+   * the only exception this can throw is std::logic_error if you call this
+   * on a Subprocess whose status is RUNNING.  Aborts on egregious
+   * violations of contract, like an out-of-band waitpid(p.pid(), 0, 0).
    */
   ProcessReturnCode wait();