Add process group leader option

Summary: Now a subprocess can be reliably made a group leader -- good for job control.

Test Plan:
unit test, checked that the pgid test worked in bash, too (only OKAY_XXX is printed)

==> XXX.sh <==
#!/bin/sh
test $(cut -d ' ' -f 5 /proc/$$/stat) == $$ && echo OKAY_XXX
./YYY.sh

==> YYY.sh <==
#!/bin/sh
test $(cut -d ' ' -f 5 /proc/$$/stat) == $$ && echo OKAY_YYY
./ZZZ.sh

==> ZZZ.sh <==
#!/bin/sh
test $(cut -d ' ' -f 5 /proc/$$/stat) == $$ && echo OKAY_ZZZ

Reviewed By: simpkins@fb.com

Subscribers: net-systems@, njormrod, folly-diffs@, simpkins

FB internal diff: D1492526

Signature: t1:1492526:1416528620:3cf98b1c1e334a7d551b2c2f3e76b1c70f07ad7c

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index 11bbc3f7bd6e62fca06ece47ac9b4df570424d44..8de5d36979b8e28b1d3cf260cd66083cc9d8b6b3 100644 (file)
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -289,6 +289,14 @@ void Subprocess::spawn(
   // child has exited and can be immediately waited for.  In all other cases,
   // we have no way of cleaning up the child.
 
+  if (options.processGroupLeader_) {
+    // This is done both in the parent and the child to avoid the race where
+    // the parent assumes that the child is a leader, but the child has not
+    // yet run setprp().  Not checking error codes since we're deliberately
+    // racing the child, which may already have run execve(), and expect to
+    // lose frequently.
+    setpgid(pid_, pid_);
+  }
   // Close writable side of the errFd pipe in the parent process
   CHECK_ERR(::close(errFds[1]));
   errFds[1] = -1;
@@ -486,6 +494,12 @@ int Subprocess::prepareChild(const Options& options,
   }
 #endif
 
+  if (options.processGroupLeader_) {
+    if (setpgrp() == -1) {
+      return errno;
+    }
+  }
+
   return 0;
 }
 

diff --git a/folly/Subprocess.h b/folly/Subprocess.h
index 65842c870bf6225a5a9933dec68702d25a620930..311b75d2219d4a67cd914d2f51b06c2951e3379b 100644 (file)
--- a/folly/Subprocess.h
+++ b/folly/Subprocess.h
@@ -205,10 +205,7 @@ class Subprocess : private boost::noncopyable {
   class Options : private boost::orable<Options> {
     friend class Subprocess;
    public:
-    Options()
-      : closeOtherFds_(false),
-        usePath_(false) {
-    }
+    Options() {}  // E.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58328
 
     /**
      * Change action for file descriptor fd.
@@ -281,6 +278,16 @@ class Subprocess : private boost::noncopyable {
     }
 #endif
 
+    /**
+     * Child will be made a process group leader when it starts. Upside: one
+     * can reliably all its kill non-daemonizing descendants.  Downside: the
+     * child will not receive Ctrl-C etc during interactive use.
+     */
+    Options& processGroupLeader() {
+      processGroupLeader_ = true;
+      return *this;
+    }
+
     /**
      * Helpful way to combine Options.
      */
@@ -289,12 +296,13 @@ class Subprocess : private boost::noncopyable {
    private:
     typedef boost::container::flat_map<int, int> FdMap;
     FdMap fdActions_;
-    bool closeOtherFds_;
-    bool usePath_;
+    bool closeOtherFds_{false};
+    bool usePath_{false};
     std::string childDir_;  // "" keeps the parent's working directory
 #if __linux__
     int parentDeathSignal_{0};
 #endif
+    bool processGroupLeader_{false};
   };
 
   static Options pipeStdin() { return Options().stdin(PIPE); }
@@ -660,6 +668,7 @@ inline Subprocess::Options& Subprocess::Options::operator|=(
   }
   closeOtherFds_ |= other.closeOtherFds_;
   usePath_ |= other.usePath_;
+  processGroupLeader_ |= other.processGroupLeader_;
   return *this;
 }
 

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index 283928de7b42b49d78b0d46fd3068ba3ded0109d..452b8626bb86101daa93ba0655816b57dc1a0b5a 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -256,6 +256,14 @@ TEST(CommunicateSubprocessTest, Duplex) {
   proc.waitChecked();
 }
 
+TEST(CommunicateSubprocessTest, ProcessGroupLeader) {
+  const auto testIsLeader = "test $(cut -d ' ' -f 5 /proc/$$/stat) == $$";
+  Subprocess nonLeader(testIsLeader);
+  EXPECT_THROW(nonLeader.waitChecked(), CalledProcessError);
+  Subprocess leader(testIsLeader, Subprocess::Options().processGroupLeader());
+  leader.waitChecked();
+}
+
 TEST(CommunicateSubprocessTest, Duplex2) {
   checkFdLeak([] {
     // Pipe 200,000 lines through sed