Commit 6b20f11e authored by Tudor Bosman's avatar Tudor Bosman Committed by Dave Watson

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
parent 309fa7e7
...@@ -731,7 +731,9 @@ void Subprocess::communicate(FdCallback readCallback, ...@@ -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)); DCHECK(!(events & POLLOUT));
if (readCallback(p.parentFd, p.childFd)) { if (readCallback(p.parentFd, p.childFd)) {
toClose.push_back(i); toClose.push_back(i);
......
...@@ -387,34 +387,51 @@ TEST(CommunicateSubprocessTest, Chatty) { ...@@ -387,34 +387,51 @@ TEST(CommunicateSubprocessTest, Chatty) {
return (wcount == lineCount); return (wcount == lineCount);
}; };
bool eofSeen = false;
auto readCallback = [&] (int pfd, int cfd) -> bool { auto readCallback = [&] (int pfd, int cfd) -> bool {
EXPECT_EQ(1, cfd); // child stdout std::string lineBuf;
EXPECT_EQ(wcount, rcount + 1);
auto expected = if (cfd != 1) {
folly::to<std::string>("a successful test ", rcount, "\n"); 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 // Not entirely kosher, we should handle partial reads, but this is
// fine for reads <= PIPE_BUF // 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); EXPECT_EQ(expected, lineBuf);
++rcount; if (wcount != lineCount) { // still more to write...
if (rcount != lineCount) {
proc.enableNotifications(0, true); proc.enableNotifications(0, true);
} }
return (rcount == lineCount); return eofSeen;
}; };
proc.communicate(readCallback, writeCallback); proc.communicate(readCallback, writeCallback);
EXPECT_EQ(lineCount, wcount); EXPECT_EQ(lineCount, wcount);
EXPECT_EQ(lineCount, rcount); EXPECT_EQ(lineCount, rcount);
EXPECT_TRUE(eofSeen);
EXPECT_EQ(0, proc.wait().exitStatus()); EXPECT_EQ(0, proc.wait().exitStatus());
}); });
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment