Fix apparent race in SubprocessTest

Summary:
The code originally contained the line

EXPECT_EQ((rcount == lineCount), r); // EOF iff at lineCount

Which asserted that, on the last iteration, EOF was being read (and thus
r was true).

This line has been causing errors in contbuild. There seem to be two
issues at play.

First, the left hand side of the EXPECT_EQ is always false. rcount is
incremented three lines down, and the (implicit) loop ends when rcount
equals lineCount. This means that, on the last iteration, rcount is
actually one less than lineCount. Hence the check is effectively
asserting that r is false.

The second issue is that r is rarely true. Empirically, a necessary but
not sufficient condition is that there be lots of other processes
running at the same time (such as when running fbconfig -r folly &&
fbmake runtests_opt). This would explain why the test passes during
development but fails in contbuild.

I am guessing that, when there are few processes running, Subprocess is
predictably ceasing to read before whichever other process yields EOF,
and that this does not always happen when there is resource contention.

As an immediate fix, I am asserting that r may only be true on the last
iteration. Could someone (cc @tudorb) who is more familiar with the
intricacies of Subprocess please check to see if this is an idiosyncrasy
of the test, or if there is actually an underlying bug.

Test Plan:
Before the change, run fbconfig -r folly && fbmake runtests_opt
repeatedly. Notice that most of the iterations yield an error from line
376 (which is consistent with contbuild error logs).

After the change, notice that the error disappears.

Reviewed By: andrewjcg@fb.com

FB internal diff: D1269124

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index 53088bb5f8882a79f76189c2465e146820cbd0a6..a9ef97007519591f1a12a2b1853a4da041905d19 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -400,7 +400,7 @@ TEST(CommunicateSubprocessTest, Chatty) {
       // fine for reads <= PIPE_BUF
       bool r = readToString(pfd, lineBuf, expected.size() + 1);
 
-      EXPECT_EQ((rcount == lineCount), r);  // EOF iff at lineCount
+      EXPECT_TRUE(!r || (rcount + 1 == lineCount)); // may read EOF at end
       EXPECT_EQ(expected, lineBuf);
 
       ++rcount;