Subprocess now does not require child processes to be reaped before destruction

Summary: To support cases where the child process may outlive the parent process.  Detaching the process is an option as well, but in that case the pid for the child process is not captured.

Reviewed By: yfeldblum

Differential Revision: D24409176

fbshipit-source-id: f74f64e3ae21baa40468c6f39ac6b4d0152db03a

folly/Subprocess.cpp

@@ -33,8 +33,6 @@
 #include <boost/container/flat_set.hpp>
 #include <boost/range/adaptors.hpp>
 
-#include <glog/logging.h>
-
 #include <folly/Conv.h>
 #include <folly/Exception.h>
 #include <folly/ScopeGuard.h>
@@ -42,6 +40,7 @@
 #include <folly/detail/AtFork.h>
 #include <folly/io/Cursor.h>
 #include <folly/lang/Assume.h>
+#include <folly/logging/xlog.h>
 #include <folly/portability/Sockets.h>
 #include <folly/portability/Stdlib.h>
 #include <folly/portability/SysSyscall.h>
@@ -193,7 +192,8 @@ Subprocess::Subprocess(
     const std::vector<std::string>& argv,
     const Options& options,
     const char* executable,
-    const std::vector<std::string>* env) {
+    const std::vector<std::string>* env)
+    : destroyOkWhileRunning_(options.allowDestructionWhileProcessRunning_) {
   if (argv.empty()) {
     throw std::invalid_argument("argv must not be empty");
   }
@@ -206,7 +206,8 @@ Subprocess::Subprocess(
 Subprocess::Subprocess(
     const std::string& cmd,
     const Options& options,
-    const std::vector<std::string>* env) {
+    const std::vector<std::string>* env)
+    : destroyOkWhileRunning_(options.allowDestructionWhileProcessRunning_) {
   if (options.usePath_) {
     throw std::invalid_argument("usePath() not allowed when running in shell");
   }
@@ -218,13 +219,18 @@ Subprocess::Subprocess(
 Subprocess Subprocess::fromExistingProcess(pid_t pid) {
   Subprocess sp;
   sp.pid_ = pid;
+  sp.destroyOkWhileRunning_ = false;
   sp.returnCode_ = ProcessReturnCode::makeRunning();
   return sp;
 }
 
 Subprocess::~Subprocess() {
-  CHECK_NE(returnCode_.state(), ProcessReturnCode::RUNNING)
-      << "Subprocess destroyed without reaping child";
+  if (!destroyOkWhileRunning_) {
+    CHECK_NE(returnCode_.state(), ProcessReturnCode::RUNNING)
+        << "Subprocess destroyed without reaping child";
+  } else if (returnCode_.state() == ProcessReturnCode::RUNNING) {
+    XLOG(DBG) << "Subprocess destroyed without reaping child process";
+  }
 }
 
 namespace {
@@ -627,8 +633,11 @@ void Subprocess::readChildErrorPipe(int pfd, const char* executable) {
     // normally, as if the child executed successfully.  If something bad
     // happened the caller should at least get a non-normal exit status from
     // the child.
-    LOG(ERROR) << "unexpected error trying to read from child error pipe "
-               << "rc=" << rc << ", errno=" << errno;
+    XLOGF(
+        ERR,
+        "unexpected error trying to read from child error pipe rc={}, errno={}",
+        rc,
+        errno);
     return;
   }
 
@@ -761,7 +770,7 @@ ProcessReturnCode Subprocess::terminateOrKill(TimeoutDuration sigtermTimeout) {
   // 3. If we are at this point, we have waited enough time after
   // sending SIGTERM, we have to use nuclear option SIGKILL to kill
   // the subprocess.
-  LOG(INFO) << "Send SIGKILL to " << pid_;
+  XLOGF(INFO, "Send SIGKILL to {}", pid_);
   kill();
   // 4. SIGKILL should kill the process otherwise there must be
   // something seriously wrong, just use blocking wait to wait for the

folly/Subprocess.h

@@ -402,6 +402,19 @@ class Subprocess {
       return *this;
     }
 
+    /**
+     * By default, if Subprocess is destroyed while the child process is
+     * still RUNNING, the destructor will log a fatal.  You can skip this
+     * behavior by setting it to true here.
+     *
+     * Note that detach()ed processes are never in RUNNING state, so this
+     * setting does not impact such processes.
+     */
+    Options& allowDestructionWhileProcessRunning(bool val) {
+      allowDestructionWhileProcessRunning_ = val;
+      return *this;
+    }
+
     /**
      * *** READ THIS WHOLE DOCBLOCK BEFORE USING ***
      *
@@ -469,6 +482,7 @@ class Subprocess {
     bool usePath_{false};
     bool processGroupLeader_{false};
     bool detach_{false};
+    bool allowDestructionWhileProcessRunning_{false};
     std::string childDir_; // "" keeps the parent's working directory
 #if defined(__linux__)
     int parentDeathSignal_{0};
@@ -941,6 +955,7 @@ class Subprocess {
 
   pid_t pid_{-1};
   ProcessReturnCode returnCode_;
+  bool destroyOkWhileRunning_{false};
 
   /**
    * Represents a pipe between this process, and the child process (or its

folly/test/SubprocessTest.cpp

@@ -188,6 +188,15 @@ TEST(SimpleSubprocessTest, waitOrTerminateOrKill_terminates_if_timeout) {
   EXPECT_EQ(SIGTERM, retCode.killSignal());
 }
 
+TEST(
+    SimpleSubprocessTest,
+    destructor_doesNotFail_ifOkToDestroyWhileProcessRunning) {
+  Subprocess proc(
+      std::vector<std::string>{"/bin/sleep", "10"},
+      Subprocess::Options().allowDestructionWhileProcessRunning(true));
+  proc.~Subprocess();
+}
+
 // This method verifies terminateOrKill shouldn't affect the exit
 // status if the process has exitted already.
 TEST(SimpleSubprocessTest, TerminateAfterProcessExit) {