Fix apparent race in SubprocessTest
authorNicholas Ormrod <njormrod@fb.com>
Mon, 14 Apr 2014 21:21:11 +0000 (14:21 -0700)
committerSara Golemon <sgolemon@fb.com>
Fri, 18 Apr 2014 19:04:14 +0000 (12:04 -0700)
commit1321eef01e58721142b3a034e1638747e251b7f7
tree75329623d30681d0f619fdbc8d56329d94a0a617
parent89c3562ec54989b0e1fbe942bf403ccbf54fbcc7
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
folly/test/SubprocessTest.cpp