Fix Chatty subprocess test, call callback on hangup

Summary:
The Chatty subprocess test incorrectly assumed that we saw EOF on the last
read from the child (that is, read() returned 0). That's not the case for two
reasons: 1. the child is allowed to stall right before it closes its stdout, in
which case we get EAGAIN, and 2. Subprocess::communicate would close the fd
without calling the read callback anyway. Fix both such things.

Test Plan: ran test

Reviewed By: njormrod@fb.com

FB internal diff: D1303215

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index 63a336a56ddd7ed12aabe9d5067fd0d26a4322f5..10740944e51f7bcbef3ca3a83f37ac7b2a50430d 100644 (file)
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -731,7 +731,9 @@ void Subprocess::communicate(FdCallback readCallback,
         }
       }
 
-      if (events & POLLIN) {
+      // Call read callback on POLLHUP, to give it a chance to read (and act
+      // on) end of file
+      if (events & (POLLIN | POLLHUP)) {
         DCHECK(!(events & POLLOUT));
         if (readCallback(p.parentFd, p.childFd)) {
           toClose.push_back(i);

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index a9ef97007519591f1a12a2b1853a4da041905d19..098b66b3aaf4b721f8caaa61e365a22a3ace2a58 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -387,34 +387,51 @@ TEST(CommunicateSubprocessTest, Chatty) {
       return (wcount == lineCount);
     };
 
+    bool eofSeen = false;
+
     auto readCallback = [&] (int pfd, int cfd) -> bool {
-      EXPECT_EQ(1, cfd);  // child stdout
-      EXPECT_EQ(wcount, rcount + 1);
+      std::string lineBuf;
 
-      auto expected =
-        folly::to<std::string>("a successful test ", rcount, "\n");
+      if (cfd != 1) {
+        EXPECT_EQ(2, cfd);
+        EXPECT_TRUE(readToString(pfd, lineBuf, 1));
+        EXPECT_EQ(0, lineBuf.size());
+        return true;
+      }
 
-      std::string lineBuf;
+      EXPECT_FALSE(eofSeen);
+
+      std::string expected;
+
+      if (rcount < lineCount) {
+        expected = folly::to<std::string>("a successful test ", rcount++, "\n");
+      }
+
+      EXPECT_EQ(wcount, rcount);
 
       // Not entirely kosher, we should handle partial reads, but this is
       // fine for reads <= PIPE_BUF
-      bool r = readToString(pfd, lineBuf, expected.size() + 1);
+      bool atEof = readToString(pfd, lineBuf, expected.size() + 1);
+      if (atEof) {
+        // EOF only expected after we finished reading
+        EXPECT_EQ(lineCount, rcount);
+        eofSeen = true;
+      }
 
-      EXPECT_TRUE(!r || (rcount + 1 == lineCount)); // may read EOF at end
       EXPECT_EQ(expected, lineBuf);
 
-      ++rcount;
-      if (rcount != lineCount) {
+      if (wcount != lineCount) {  // still more to write...
         proc.enableNotifications(0, true);
       }
 
-      return (rcount == lineCount);
+      return eofSeen;
     };
 
     proc.communicate(readCallback, writeCallback);
 
     EXPECT_EQ(lineCount, wcount);
     EXPECT_EQ(lineCount, rcount);
+    EXPECT_TRUE(eofSeen);
 
     EXPECT_EQ(0, proc.wait().exitStatus());
   });