FBProcessIO: Attachment only once, detachment allowed multiple times

Summary:
`-[FBProcessIO detach]` should be called at least one time once a process has exited. However, the current implementation for pipe-based consumers (data consumers, block consumers etc) if `-[FBProcessIO detach]` will error out and bail early another detachment is occurring. That started detachment may finish after the error condition, if the caller is listening for the errored-out attachment, then detachment errors can occur before the detachment has finished elsewhere

This identifies another race condition; D29194120 (dac5809e41) fixes problems in `FBTask` where the properties on it do not wait until IO is drained. However, there's still a problem that relates to `-[FBProcessIO detach]`.

1) A process is running, with the process futures all wrapped with detachment handling.
2) The `-[FBProcessIO detach]` is in sequence for `statLoc`, `exitCode` and `signal`. This causes each output to teardown.
3) The same detachment code is called for each of stdIn, stdErr, stdOut.
4) The teardown for `statLoc`'s `stdOut` teardown can succeed first
5) The teardown for `exitCode`'s `stdOut` teardown then fails and the error is handled
6) However, the real teardown in #4 is still pending. This means that `exitCode` can have resolved, before the teardown has actually occurred, which we explicitly wanted to avoid

There was a further problem with the `FBProcessIO` class. Since the internal queue used for serialisation was a concurrent queue, all of the serialisation that was necessary to prevent attachments and detachments from fighting with each other was effectively redundant. This could have resulted in crashes and other indeterminate states if attachments and detachments ran concurrently with themselves or each other.

Reviewed By: jbardini

Differential Revision: D29195990

fbshipit-source-id: 50722ce1eb4293ac6cf6183459d169b42e0c40e7

FBControlCore/Utility/FBProcessIO.h

@@ -102,6 +102,7 @@ The FBProcessOutput for stdout.
  Attach to all the streams, returning the composite attachment for file descriptors.
  Will error if any of the stream attachments error.
  If any of the stream attachments error, then any succeeding attachments will detach.
+ This should only be called once. Calling attach more than once per instance will fail.
  */
 - (FBFuture<FBProcessIOAttachment *> *)attach;
 
@@ -114,6 +115,7 @@ The FBProcessOutput for stdout.
 
 /**
  Detach from all the streams.
+ This may be called multiple times, the underlying streams will only detach once per instance.
  */
 - (FBFuture<NSNull *> *)detach;
 

FBControlCore/Utility/FBProcessIO.m

@@ -8,6 +8,7 @@
 #import "FBProcessIO.h"
 
 #import "FBProcessStream.h"
+#import "FBControlCoreError.h"
 
 @implementation FBProcessIOAttachment
 
@@ -44,6 +45,13 @@
 
 @end
 
+@interface FBProcessIO ()
+
+@property (nonatomic, assign, readwrite) BOOL attached;
+@property (nonatomic, nullable, readwrite) FBMutableFuture<NSNull *> *detachment;
+
+@end
+
 @implementation FBProcessIO
 
 - (instancetype)initWithStdIn:(nullable FBProcessInput *)stdIn stdOut:(nullable FBProcessOutput *)stdOut stdErr:(nullable FBProcessOutput *)stdErr
@@ -56,7 +64,7 @@
   _stdIn = stdIn;
   _stdOut = stdOut;
   _stdErr = stdErr;
-  _queue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0);
+  _queue = dispatch_queue_create("com.facebook.FBControlCore.FBProcessIO", DISPATCH_QUEUE_SERIAL);
 
   return self;
 }
@@ -70,12 +78,15 @@
 
 - (FBFuture<FBProcessIOAttachment *> *)attach
 {
-  return [[FBFuture
-    futureWithFutures:@[
+  return [[[self
+    startExclusiveAttachment]
+    onQueue:self.queue fmap:^(id _) {
+      return [FBFuture futureWithFutures:@[
         [self wrapAttachment:self.stdIn],
         [self wrapAttachment:self.stdOut],
         [self wrapAttachment:self.stdErr],
-    ]]
+      ]];
+    }]
     onQueue:self.queue fmap:^ FBFuture * (NSArray<id> *attachments) {
       // Mount all the relevant std streams.
       id stdIn = attachments[0];
@@ -106,11 +117,14 @@
 
 - (FBFuture<FBProcessFileAttachment *> *)attachViaFile
 {
-  return [[FBFuture
-    futureWithFutures:@[
+  return [[[self
+    startExclusiveAttachment]
+    onQueue:self.queue fmap:^(id _) {
+      return [FBFuture futureWithFutures:@[
         [self wrapFileAttachment:self.stdOut],
         [self wrapFileAttachment:self.stdErr],
-    ]]
+      ]];
+    }]
     onQueue:self.queue fmap:^ FBFuture * (NSArray<id> *attachments) {
       id stdOut = attachments[0];
       if ([stdOut isKindOfClass:NSError.class]) {
@@ -132,6 +146,42 @@
 }
 
 - (FBFuture<NSNull *> *)detach
+{
+  return [FBFuture
+    onQueue:self.queue resolve:^ FBFuture<NSNull *> * {
+      if (self.attached == NO) {
+        return [[FBControlCoreError
+          describeFormat:@"Cannot detach when -attach has not been called"]
+          failFuture];
+      }
+      FBMutableFuture<NSNull *> *detachment = self.detachment;
+      if (detachment) {
+        return detachment;
+      }
+      detachment = FBMutableFuture.future;
+      self.detachment = detachment;
+      [detachment resolveFromFuture:[self performDetachment]];
+      return detachment;
+    }];
+}
+
+#pragma mark Private
+
+- (FBFuture<NSNull *> *)startExclusiveAttachment
+{
+  return [FBFuture
+    onQueue:self.queue resolve:^ FBFuture<NSNull *> * {
+      if (self.attached) {
+        return [[FBControlCoreError
+          describeFormat:@"Cannot -attach twice"]
+          failFuture];
+      }
+      self.attached = YES;
+      return FBFuture.empty;
+    }];
+}
+
+- (FBFuture<NSNull *> *)performDetachment
 {
   return [[FBFuture
     futureWithFutures:@[
@@ -150,8 +200,6 @@
     }];
 }
 
-#pragma mark Private
-
 - (FBFuture *)wrapAttachment:(id<FBStandardStream>)stream
 {
   if (!stream) {

FBControlCoreTests/Tests/Integration/FBProcessIOTests.m

@@ -0,0 +1,82 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#import <XCTest/XCTest.h>
+
+#import <FBControlCore/FBControlCore.h>
+
+#import <sys/types.h>
+#import <sys/stat.h>
+
+@interface FBProcessIOTests : XCTestCase
+
+@end
+
+@implementation FBProcessIOTests
+
+- (void)testDetachmentMultipleTimesIsPermitted
+{
+  id<FBDataConsumer, FBDataConsumerLifecycle> stdInConsumer = FBDataBuffer.consumableBuffer;
+  id<FBDataConsumer, FBDataConsumerLifecycle> stdOutConsumer = FBDataBuffer.consumableBuffer;
+  FBProcessIO *io = [[FBProcessIO alloc] initWithStdIn:nil stdOut:[FBProcessOutput outputForDataConsumer:stdInConsumer] stdErr:[FBProcessOutput outputForDataConsumer:stdOutConsumer]];
+
+  NSError *error = nil;
+  BOOL success = [[io attach] await:&error] != nil;
+  XCTAssertNil(error);
+  XCTAssertTrue(success);
+
+  dispatch_queue_t concurrentQueue = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0);
+  dispatch_group_t group = dispatch_group_create();
+  __block FBFuture<NSNull *> *first;
+  __block FBFuture<NSNull *> *second;
+  __block FBFuture<NSNull *> *third;
+  __block FBFuture<NSNull *> *fourth;
+
+  dispatch_group_async(group, concurrentQueue, ^{
+    first = [io detach];
+  });
+  dispatch_group_async(group, concurrentQueue, ^{
+    second = [io detach];
+  });
+  dispatch_group_async(group, concurrentQueue, ^{
+    third = [io detach];
+  });
+  dispatch_group_async(group, concurrentQueue, ^{
+    fourth = [io detach];
+  });
+  dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
+
+  for (FBFuture<NSNull *> *attempt in @[first, second, third, fourth]) {
+    success = [attempt await:&error] != nil;
+    XCTAssertNil(error);
+    XCTAssertTrue(success);
+    XCTAssertTrue(stdInConsumer.finishedConsuming.hasCompleted);
+  }
+}
+
+- (void)testMultipleAttachmentIsNotPermitted
+{
+  id<FBDataConsumer, FBDataConsumerLifecycle> stdInConsumer = FBDataBuffer.consumableBuffer;
+  id<FBDataConsumer, FBDataConsumerLifecycle> stdOutConsumer = FBDataBuffer.consumableBuffer;
+  FBProcessIO *io = [[FBProcessIO alloc] initWithStdIn:nil stdOut:[FBProcessOutput outputForDataConsumer:stdInConsumer] stdErr:[FBProcessOutput outputForDataConsumer:stdOutConsumer]];
+
+  NSError *error = nil;
+  BOOL success = [[io attach] await:&error] != nil;
+  XCTAssertNil(error);
+  XCTAssertTrue(success);
+
+  success = [[io attach] await:&error] != nil;
+  XCTAssertNotNil(error);
+  XCTAssertFalse(success);
+
+  error = nil;
+  success = [[io detach] await:&error] != nil;
+  XCTAssertNil(error);
+  XCTAssertTrue(success);
+}
+
+@end

FBSimulatorControl.xcodeproj/project.pbxproj

@@ -227,6 +227,7 @@
 		AA6A3B431CC1597000E016C4 /* FBSimulatorTerminationStrategy.h in Headers */ = {isa = PBXBuildFile; fileRef = AA6A3B391CC1597000E016C4 /* FBSimulatorTerminationStrategy.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		AA6A3B441CC1597000E016C4 /* FBSimulatorTerminationStrategy.m in Sources */ = {isa = PBXBuildFile; fileRef = AA6A3B3A1CC1597000E016C4 /* FBSimulatorTerminationStrategy.m */; };
 		AA6B1DD21FC5FCFA009DDDAE /* FBDataBufferTests.m in Sources */ = {isa = PBXBuildFile; fileRef = AA6B1DD11FC5FCFA009DDDAE /* FBDataBufferTests.m */; };
+		AA6C68A4267B89C100EB975D /* FBProcessIOTests.m in Sources */ = {isa = PBXBuildFile; fileRef = AA6C68A3267B89C100EB975D /* FBProcessIOTests.m */; };
 		AA6F22441C916A31009F5CE4 /* photo0.png in Resources */ = {isa = PBXBuildFile; fileRef = AA6F22411C916A31009F5CE4 /* photo0.png */; };
 		AA6F22451C916A31009F5CE4 /* simulator_system.log in Resources */ = {isa = PBXBuildFile; fileRef = AA6F22421C916A31009F5CE4 /* simulator_system.log */; };
 		AA6F22461C916A31009F5CE4 /* tree.json in Resources */ = {isa = PBXBuildFile; fileRef = AA6F22431C916A31009F5CE4 /* tree.json */; };
@@ -1059,6 +1060,7 @@
 		AA6A3B391CC1597000E016C4 /* FBSimulatorTerminationStrategy.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = FBSimulatorTerminationStrategy.h; sourceTree = "<group>"; };
 		AA6A3B3A1CC1597000E016C4 /* FBSimulatorTerminationStrategy.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = FBSimulatorTerminationStrategy.m; sourceTree = "<group>"; };
 		AA6B1DD11FC5FCFA009DDDAE /* FBDataBufferTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = FBDataBufferTests.m; sourceTree = "<group>"; };
+		AA6C68A3267B89C100EB975D /* FBProcessIOTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = FBProcessIOTests.m; sourceTree = "<group>"; };
 		AA6F22411C916A31009F5CE4 /* photo0.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = photo0.png; sourceTree = "<group>"; };
 		AA6F22421C916A31009F5CE4 /* simulator_system.log */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = simulator_system.log; sourceTree = "<group>"; };
 		AA6F22431C916A31009F5CE4 /* tree.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = tree.json; sourceTree = "<group>"; };
@@ -1664,6 +1666,7 @@
 			children = (
 				AA2076D11F0B779B001F180C /* FBFileReaderTests.m */,
 				AA2076CF1F0B76AF001F180C /* FBFileWriterTests.m */,
+				AA6C68A3267B89C100EB975D /* FBProcessIOTests.m */,
 				AA274282204546F800CFAC3B /* FBProcessStreamTests.m */,
 				AA2076A71F0B7541001F180C /* FBTaskTests.m */,
 			);
@@ -3701,6 +3704,7 @@
 				AA2076BB1F0B7542001F180C /* FBiOSTargetConfigurationTests.m in Sources */,
 				AA2076B91F0B7542001F180C /* FBTaskTests.m in Sources */,
 				AA9319B822B78FB800C68F65 /* FBBinaryDescriptorTests.m in Sources */,
+				AA6C68A4267B89C100EB975D /* FBProcessIOTests.m in Sources */,
 				AA2076BD1F0B7542001F180C /* FBCrashLogInfoTests.m in Sources */,
 				EE87FA432008D906002716FE /* AXTraitsTest.m in Sources */,
 				AA08487E1F3F49D600A4BA60 /* FBFutureTests.m in Sources */,