Fix Cancellation Race in FBProcessStream

Summary:
In `FBProcessStream` we will detach, regardless of whether there is work to be done. In this case it is better to wait for all of the data to be drained from the pipe and then cancel

The bad case is:
1) Process Reader is reading output
2) Task Terminates
3) detach is called
4) Process Reader is stopped
5) io is cancelled, with data still to be read

This case relies on the pipe dying naturally after a subprocess exits

Reviewed By: asm89

Differential Revision: D8764892

fbshipit-source-id: 5961915f9b6e9f562953567682aeb8be25e75d3b

FBControlCore/Utility/FBProcessStream.m

@@ -16,6 +16,8 @@
 #import "FBFileWriter.h"
 #import "FBFileReader.h"
 
+static NSTimeInterval ProcessDetachDrainTimeout = 4;
+
 #pragma mark FBProcessFileOutput
 
 @interface FBProcessFileOutput_DirectToFile : NSObject <FBProcessFileOutput>
@@ -499,7 +501,11 @@ - (instancetype)initWithConsumer:(id<FBFileConsumer>)consumer
 
 - (FBFuture<NSNull *> *)detach
 {
-  return [self.reader.stopReading
+  return [[[self.reader.completed
+    timeout:ProcessDetachDrainTimeout waitingFor:@"Process Reading to Finish"]
+    onQueue:self.workQueue chain:^(FBFuture *_) {
+      return [self.reader stopReading];
+    }]
     onQueue:self.workQueue chain:^(FBFuture *future) {
       NSPipe *pipe = self.pipe;
       [pipe.fileHandleForWriting closeFile];

FBControlCoreTests/Doubles/FBControlCoreLoggerDouble.m

@@ -53,7 +53,7 @@ - (NSString *)name
 
 - (FBControlCoreLogLevel)level
 {
-  return FBControlCoreLogLevelUnknown;
+  return FBControlCoreLogLevelMultiple;
 }
 
 @end

FBControlCoreTests/Tests/Unit/FBFutureContextManagerTests.m

@@ -49,8 +49,8 @@ - (void)testSingleAquire
   XCTAssertNil(error);
   XCTAssertEqualObjects(value, @123);
 
-  XCTAssertEqual(self.prepareCalled, 1);
-  XCTAssertEqual(self.teardownCalled, 1);
+  XCTAssertEqual(self.prepareCalled, 1u);
+  XCTAssertEqual(self.teardownCalled, 1u);
 }
 
 - (void)testSequentialAquire
@@ -68,8 +68,8 @@ - (void)testSequentialAquire
   XCTAssertNil(error);
   XCTAssertEqualObjects(value, @0);
 
-  XCTAssertEqual(self.prepareCalled, 1);
-  XCTAssertEqual(self.teardownCalled, 1);
+  XCTAssertEqual(self.prepareCalled, 1u);
+  XCTAssertEqual(self.teardownCalled, 1u);
 
   FBFuture *future1 = [[manager
     utilizeWithPurpose:@"A Test"]
@@ -80,8 +80,8 @@ - (void)testSequentialAquire
   XCTAssertNil(error);
   XCTAssertEqualObjects(value, @1);
 
-  XCTAssertEqual(self.prepareCalled, 2);
-  XCTAssertEqual(self.teardownCalled, 2);
+  XCTAssertEqual(self.prepareCalled, 2u);
+  XCTAssertEqual(self.teardownCalled, 2u);
 }
 
 - (void)testSequentialAquireWithCooloff
@@ -100,8 +100,8 @@ - (void)testSequentialAquireWithCooloff
   XCTAssertNil(error);
   XCTAssertEqualObjects(value, @0);
 
-  XCTAssertEqual(self.prepareCalled, 1);
-  XCTAssertEqual(self.teardownCalled, 0);
+  XCTAssertEqual(self.prepareCalled, 1u);
+  XCTAssertEqual(self.teardownCalled, 0u);
 
   FBFuture *future1 = [[manager
     utilizeWithPurpose:@"A Test"]
@@ -112,13 +112,13 @@ - (void)testSequentialAquireWithCooloff
   XCTAssertNil(error);
   XCTAssertEqualObjects(value, @1);
 
-  XCTAssertEqual(self.prepareCalled, 1);
-  XCTAssertEqual(self.teardownCalled, 0);
+  XCTAssertEqual(self.prepareCalled, 1u);
+  XCTAssertEqual(self.teardownCalled, 0u);
 
   [[FBFuture futureWithDelay:0.25 future:[FBFuture futureWithResult:NSNull.null]] await:nil];
 
-  XCTAssertEqual(self.prepareCalled, 1);
-  XCTAssertEqual(self.teardownCalled, 1);
+  XCTAssertEqual(self.prepareCalled, 1u);
+  XCTAssertEqual(self.teardownCalled, 1u);
 }
 
 - (void)testConcurrentAquireOnlyPreparesOnce
@@ -159,8 +159,8 @@ - (void)testConcurrentAquireOnlyPreparesOnce
   XCTAssertNil(error);
   XCTAssertEqualObjects(value, (@[@0, @1, @2]));
 
-  XCTAssertEqual(self.prepareCalled, 1);
-  XCTAssertEqual(self.teardownCalled, 1);
+  XCTAssertEqual(self.prepareCalled, 1u);
+  XCTAssertEqual(self.teardownCalled, 1u);
 }
 
 - (void)testImmediateAquireAndRelease