Fix IO Channel disposal in FBFileWriter

Summary: See the comments for this one

Reviewed By: c-ryan747

Differential Revision: D13692550

fbshipit-source-id: dedd7bcf17a659bd468c207902cea65fdb1b486b

FBControlCore/Utility/FBFileWriter.m

@@ -234,13 +234,26 @@
   NSParameterAssert(!self.io);
 
   // If there is an error creating the IO Object, the errorCode will be delivered asynchronously.
-  NSFileHandle *fileHandle = self.fileHandle;
-  self.io = dispatch_io_create(DISPATCH_IO_STREAM, fileHandle.fileDescriptor, self.writeQueue, ^(int errorCode) {
-    [self ioChannelDidCloseWithError:errorCode];
+  // Having a self -> IO -> self cycle shouldn't be a problem in theory, since the cleanup handler should get when IO is done.
+  // However, it appears that having the cycle in place here means that the cleanup handler is *never* called in the following circumstance:
+  // 1) Pipe of FD14 is created.
+  // 2) A writer is created for this pipe
+  // 3) Data is written to this writer
+  // 4) `consumeEndOfFile` is called and subsequently dispatch_io_close.
+  // 5) The cleanup handler is called and subsequently the FD closed and IO channel disposed of via nil-ification.
+  // 6) Pipe FD14 is torn down.
+  // 7) A new Pipe resolving to FD14 is created.
+  // 8) Data is written to this writer
+  // 9) `consumeEndOfFile` is called and subsequently dispatch_io_close.
+  // 10) The cleanup handler is *never* called and the FD is therefore never closed.
+  // This isn't a problem in practice if different FDs are splayed, but repeating FDs representing different dispatch channels will cause this problem.
+  __weak typeof(self) weakSelf = self;
+  self.io = dispatch_io_create(DISPATCH_IO_STREAM, self.fileHandle.fileDescriptor, self.writeQueue, ^(int errorCode) {
+    [weakSelf ioChannelDidCloseWithError:errorCode];
   });
   if (!self.io) {
     return [[FBControlCoreError
-      describeFormat:@"A IO Channel could not be created for fd %d", fileHandle.fileDescriptor]
+      describeFormat:@"A IO Channel could not be created for fd %d", self.fileHandle.fileDescriptor]
       failBool:error];
   }
 

FBControlCoreTests/Fixtures/FBControlCoreFixtures.h

@@ -53,6 +53,11 @@
  */
 + (NSString *)agentCrashPathWithCustomDeviceSet;
 
+/**
+ All of the above, in a directory
+ */
++ (NSString *)bundleResource;
+
 @end
 
 @interface XCTestCase (FBControlCoreFixtures)

FBControlCoreTests/Fixtures/FBControlCoreFixtures.m

@@ -48,6 +48,11 @@
   return [[NSBundle bundleForClass:self] pathForResource:@"agent_custom_set" ofType:@"crash"];
 }
 
++ (NSString *)bundleResource
+{
+  return [NSBundle bundleForClass:self].resourcePath;
+}
+
 @end
 
 @implementation XCTestCase (FBControlCoreFixtures)

FBControlCoreTests/Tests/Integration/FBTaskTests.m

@@ -324,4 +324,42 @@
   XCTAssertEqual(task.completed.state, FBFutureStateDone);
 }
 
+- (void)testPipingInputToSuccessivelyRunTasksSucceeds
+{
+  NSString *tarSource = [NSTemporaryDirectory() stringByAppendingPathComponent:[NSString stringWithFormat:@"%@.tar.gz", NSUUID.UUID.UUIDString]];
+  NSString *tarDestination = [tarSource stringByAppendingString:@".destination"];
+
+  NSError *error = nil;
+  BOOL success = [NSFileManager.defaultManager createDirectoryAtPath:tarDestination withIntermediateDirectories:YES attributes:nil error:&error];
+  XCTAssertNil(error);
+  XCTAssertTrue(success);
+
+  success = [[[[[[[FBTaskBuilder
+    withLaunchPath:@"/usr/bin/tar"]
+    withArguments:@[@"-zcvf", tarSource, FBControlCoreFixtures.bundleResource]]
+    withStdOutToDevNull]
+    withStdErrToDevNull]
+    runUntilCompletion]
+    mapReplace:NSNull.null]
+    await:&error] != nil;
+  XCTAssertNil(error);
+  XCTAssertTrue(success);
+
+  NSData *tarData = [NSData dataWithContentsOfFile:tarSource];
+
+  for (NSUInteger count = 0; count < 10; count++) {
+    success = [[[[[[[[FBTaskBuilder
+      withLaunchPath:@"/usr/bin/tar"]
+      withArguments:@[@"-C", tarDestination, @"-zxpf", @"-"]]
+      withStdInFromData:tarData]
+      withStdOutToDevNull]
+      withStdErrToDevNull]
+      runUntilCompletion]
+      mapReplace:NSNull.null]
+      await:&error] != nil;
+    XCTAssertNil(error);
+    XCTAssertTrue(success);
+  }
+}
+
 @end