Revert "Fix up a race condition in GTMSimpleWorkerThread"

This reverts commit 585330a68d00c4d76927ff7bf4829471944358ab.

3 files changed, 319 insertions, 105 deletions

diff --git a/Foundation/GTMNSThread+Blocks.h b/Foundation/GTMNSThread+Blocks.h
index 4240b4c..17bfbc7 100644
--- a/Foundation/GTMNSThread+Blocks.h
+++ b/Foundation/GTMNSThread+Blocks.h
@@ -40,5 +40,13 @@
 
 // A simple thread that does nothing but handle performBlock and
 // performSelector calls.
-@interface GTMSimpleWorkerThread : NSThread
+@interface GTMSimpleWorkerThread : NSThread {
+ @private
+  CFRunLoopRef runLoop_;
+  NSConditionLock *runLock_;
+}
+
+// Will stop the thread, blocking till the thread exits.
+- (void)stop;
+
 @end
diff --git a/Foundation/GTMNSThread+Blocks.m b/Foundation/GTMNSThread+Blocks.m
index e754f6f..8318193 100644
--- a/Foundation/GTMNSThread+Blocks.m
+++ b/Foundation/GTMNSThread+Blocks.m
@@ -53,76 +53,220 @@
 
 #endif  // NS_BLOCKS_AVAILABLE
 
-@implementation GTMSimpleWorkerThread {
-  NSLock *sourceLock_;
-  CFRunLoopSourceRef source_;  // Protected by sourceLock_
-  CFRunLoopRef cfRunLoop_;
-}
+enum {
+  kGTMSimpleThreadInitialized = 0,
+  kGTMSimpleThreadStarting,
+  kGTMSimpleThreadRunning,
+  kGTMSimpleThreadCancel,
+  kGTMSimpleThreadFinished,
+};
+
+@implementation GTMSimpleWorkerThread
 
-static void RunLoopContextEmptyFunc(void *info) {
-  // Empty because the source is used solely for signalling.
-  // The documentation for CFRunLoopSourceContext does not
-  // make it clear if you can have a null perform method.
+- (id)init {
+  if ((self = [super init])) {
+    runLock_ =
+        [[NSConditionLock alloc] initWithCondition:kGTMSimpleThreadInitialized];
+  }
+  return self;
 }
 
-- (void)main {
-  NSRunLoop *nsRunLoop = [NSRunLoop currentRunLoop];
-  {  // Braces are just to denote what is protected by sourceLock_
-    [sourceLock_ lock];
-    cfRunLoop_ = [nsRunLoop getCFRunLoop];
-    CFRetain(cfRunLoop_);
-    CFRunLoopSourceContext context = {0};
-    context.perform = RunLoopContextEmptyFunc;
-    source_ = CFRunLoopSourceCreate(NULL, 0, &context);
-    CFRunLoopAddSource(cfRunLoop_, source_, kCFRunLoopCommonModes);
-    [sourceLock_ unlock];
+- (void)dealloc {
+  if ([self isExecuting]) {
+    [self stop];
   }
-  while (true) {
-    BOOL cancelled = [self isCancelled];
-    if (cancelled) {
-      break;
-    }
-    BOOL ranLoop = [nsRunLoop runMode:NSDefaultRunLoopMode
-                           beforeDate:[NSDate distantFuture]];
-    if (!ranLoop) {
-      break;
-    }
+  [runLock_ release];
+  [super dealloc];
+}
+
+- (void)setThreadDebuggerName:(NSString *)name {
+  if ([name length]) {
+    pthread_setname_np([name UTF8String]);
+  } else {
+    pthread_setname_np("");
   }
 }
 
-- (void)dealloc {
-  if (cfRunLoop_) {
-    CFRelease(cfRunLoop_);
+- (void)main {
+  [runLock_ lock];
+  if ([runLock_ condition] != kGTMSimpleThreadStarting) {
+    // Don't start, we're already cancelled or we've been started twice.
+    [runLock_ unlock];
+    return;
   }
-  if (source_) {
-    CFRelease(source_);
+
+  // Give ourself an autopool
+  NSAutoreleasePool *localPool = [[NSAutoreleasePool alloc] init];
+
+  // Expose the current runloop so other threads can stop (but see caveat
+  // below).
+  NSRunLoop *loop = [NSRunLoop currentRunLoop];
+  runLoop_ = [loop getCFRunLoop];
+  if (runLoop_) CFRetain(runLoop_);  // NULL check is pure paranoia.
+
+  // Add a port to the runloop so that it stays alive. Without a port attached
+  // to it, a runloop will immediately return when you call run on it.
+  [loop addPort:[NSPort port] forMode:NSDefaultRunLoopMode];
+
+  // Name ourself
+  [self setThreadDebuggerName:[self name]];
+
+  // We're officially running.
+  [runLock_ unlockWithCondition:kGTMSimpleThreadRunning];
+
+  while (![self isCancelled] &&
+         [runLock_ tryLockWhenCondition:kGTMSimpleThreadRunning]) {
+    [runLock_ unlock];
+    // We can't control nesting of runloops, so we spin with a short timeout. If
+    // another thread cancels us the CFRunloopStop() we might get it right away,
+    // if there is no nesting, otherwise our timeout will still get us to exit
+    // in reasonable time.
+    [loop runMode:NSDefaultRunLoopMode
+         beforeDate:[NSDate dateWithTimeIntervalSinceNow:1.0]];
+    [localPool drain];
+    localPool = [[NSAutoreleasePool alloc] init];
   }
-  [super dealloc];
+
+  // Exit
+  [runLock_ lock];
+  [localPool drain];
+  if (runLoop_) CFRelease(runLoop_);
+  runLoop_ = NULL;
+  [runLock_ unlockWithCondition:kGTMSimpleThreadFinished];
 }
 
 - (void)start {
-  // Protect lock in case we are "started" twice in different threads.
-  // NSThread has no documentation regarding the safety of this, so
-  // making safe by default.
-  @synchronized (self) {
-    if (sourceLock_) {
-      return;
-    }
-    sourceLock_ = [[NSLock alloc] init];
+  // Before we start the thread we need to make sure its not already running
+  // and that the lock is past kGTMSimpleThreadInitialized so an immediate
+  // stop is safe.
+  [runLock_ lock];
+  if ([runLock_ condition] != kGTMSimpleThreadInitialized) {
+    [runLock_ unlock];
+    return;
   }
+  [runLock_ unlockWithCondition:kGTMSimpleThreadStarting];
   [super start];
 }
 
 - (void)cancel {
+  // NSThread appears to not propagate [... isCancelled] to our thread in
+  // this subclass, so we'll let super know and then use our condition lock.
   [super cancel];
-  {  // Braces are just to denote what is protected by sourceLock_
-    [sourceLock_ lock];
-    if (source_) {
-      CFRunLoopSourceSignal(source_);
-      CFRunLoopWakeUp(cfRunLoop_);
-    }
-    [sourceLock_ unlock];
+  [runLock_ lock];
+  switch ([runLock_ condition]) {
+    case kGTMSimpleThreadInitialized:
+    case kGTMSimpleThreadStarting:
+      // Cancelled before we started? Transition straight to finished.
+      [runLock_ unlockWithCondition:kGTMSimpleThreadFinished];
+      return;
+    case kGTMSimpleThreadRunning:
+      // If the thread has exited without changing lock state we detect that
+      // here. Note this is a direct call to [super isExecuting] to prevent
+      // deadlock on |runLock_| in [self isExecuting].
+      if (![super isExecuting]) {
+        // Thread died in some unanticipated way, clean up on its behalf.
+        if (runLoop_) {
+          CFRelease(runLoop_);
+          runLoop_ = NULL;
+        }
+        [runLock_ unlockWithCondition:kGTMSimpleThreadFinished];
+        return;
+      } else {
+        // We need to cancel the running loop. We'd like to stop the runloop
+        // right now if we can (but see the caveat above about nested runloops).
+        if (runLoop_) CFRunLoopStop(runLoop_);
+        [runLock_ unlockWithCondition:kGTMSimpleThreadCancel];
+        return;
+      }
+    case kGTMSimpleThreadCancel:
+    case kGTMSimpleThreadFinished:
+      // Already cancelled or finished. There's an outside chance the thread
+      // will have died now (imagine a [... dealloc] that calls pthread_exit())
+      // but we'll ignore those cases.
+      [runLock_ unlock];
+      return;
+  }
+}
+
+- (void)stop {
+  // Cancel does the heavy lifting...
+  [self cancel];
+
+  // If we're the current thread then the stop was called from within our
+  // own runloop and we need to return control now. [... main] will handle
+  // the shutdown on its own.
+  if ([[NSThread currentThread] isEqual:self]) return;
+
+  // From all other threads block till we're finished. Note that [... cancel]
+  // handles ensuring we will either already be in this state or transition
+  // there after thread exit.
+  [runLock_ lockWhenCondition:kGTMSimpleThreadFinished];
+  [runLock_ unlock];
+
+  // We could still be waiting for thread teardown at this point (lock is in
+  // the right state, but thread is not quite torn down), so spin till we say
+  // execution is complete (our implementation checks superclass).
+  while ([self isExecuting]) {
+    usleep(10);
+  }
+}
+
+- (void)setName:(NSString *)name {
+  if ([self isExecuting]) {
+    [self performSelector:@selector(setThreadDebuggerName:)
+                 onThread:self
+               withObject:name
+            waitUntilDone:YES];
+  }
+  [super setName:name];
+}
+
+- (BOOL)isCancelled {
+  if ([super isCancelled]) return YES;
+  BOOL cancelled = NO;
+  [runLock_ lock];
+  if ([runLock_ condition] == kGTMSimpleThreadCancel) {
+    cancelled = YES;
+  }
+  [runLock_ unlock];
+  return cancelled;
+}
+
+- (BOOL)isExecuting {
+  if ([super isExecuting]) return YES;
+  [runLock_ lock];
+  switch ([runLock_ condition]) {
+    case kGTMSimpleThreadStarting:
+      // While starting we may not be executing yet, but we'll pretend we are.
+      [runLock_ unlock];
+      return YES;
+    case kGTMSimpleThreadCancel:
+    case kGTMSimpleThreadRunning:
+      // Both of these imply we're running, but [super isExecuting] failed,
+      // so the thread died for other reasons. Clean up.
+      if (runLoop_) {
+        CFRelease(runLoop_);
+        runLoop_ = NULL;
+      }
+      [runLock_ unlockWithCondition:kGTMSimpleThreadFinished];
+      break;
+    default:
+      [runLock_ unlock];
+      break;
+  }
+  return NO;
+}
+
+- (BOOL)isFinished {
+  if ([super isFinished]) return YES;
+  if ([self isExecuting]) return NO;  // Will clean up dead thread.
+  BOOL finished = NO;
+  [runLock_ lock];
+  if ([runLock_ condition] == kGTMSimpleThreadFinished) {
+    finished = YES;
   }
+  [runLock_ unlock];
+  return finished;
 }
 
 @end
diff --git a/Foundation/GTMNSThread+BlocksTest.m b/Foundation/GTMNSThread+BlocksTest.m
index ff1a1a0..4b685fd 100644
--- a/Foundation/GTMNSThread+BlocksTest.m
+++ b/Foundation/GTMNSThread+BlocksTest.m
@@ -20,7 +20,7 @@
 #import "GTMSenTestCase.h"
 #import "GTMNSThread+Blocks.h"
 
-static const NSTimeInterval kTestTimeout = 5;
+#import "GTMFoundationUnitTestingUtilities.h"
 
 @interface GTMNSThread_BlocksTest : GTMTestCase {
  @private
@@ -36,46 +36,60 @@ static const NSTimeInterval kTestTimeout = 5;
 }
 
 - (void)tearDown {
-  [workerThread_ cancel];
+  [workerThread_ stop];
   [workerThread_ release];
 }
 
 - (void)testPerformBlockOnCurrentThread {
   NSThread *currentThread = [NSThread currentThread];
+
+  GTMUnitTestingBooleanRunLoopContext *context =
+      [GTMUnitTestingBooleanRunLoopContext context];
   __block NSThread *runThread = nil;
 
   // Straight block runs right away (no runloop spin)
+  runThread = nil;
+  [context setShouldStop:NO];
   [currentThread gtm_performBlock:^{
     runThread = [NSThread currentThread];
+    [context setShouldStop:YES];
   }];
   XCTAssertEqualObjects(runThread, currentThread);
+  XCTAssertTrue([context shouldStop]);
 
   // Block with waiting runs immediately as well.
   runThread = nil;
+  [context setShouldStop:NO];
   [currentThread gtm_performWaitingUntilDone:YES block:^{
     runThread = [NSThread currentThread];
+    [context setShouldStop:YES];
   }];
   XCTAssertEqualObjects(runThread, currentThread);
+  XCTAssertTrue([context shouldStop]);
 
   // Block without waiting requires a runloop spin.
   runThread = nil;
-  XCTestExpectation *expectation = [self expectationWithDescription:@"BlockRan"];
+  [context setShouldStop:NO];
   [currentThread gtm_performWaitingUntilDone:NO block:^{
     runThread = [NSThread currentThread];
-    [expectation fulfill];
+    [context setShouldStop:YES];
   }];
-  [self waitForExpectationsWithTimeout:kTestTimeout handler:NULL];
+  XCTAssertTrue([[NSRunLoop currentRunLoop]
+                 gtm_runUpToSixtySecondsWithContext:context]);
   XCTAssertEqualObjects(runThread, currentThread);
+  XCTAssertTrue([context shouldStop]);
 }
 
 - (void)testPerformBlockInBackground {
-  XCTestExpectation *expectation = [self expectationWithDescription:@"BlockRan"];
+  GTMUnitTestingBooleanRunLoopContext *context =
+      [GTMUnitTestingBooleanRunLoopContext context];
   __block NSThread *runThread = nil;
   [NSThread gtm_performBlockInBackground:^{
     runThread = [NSThread currentThread];
-    [expectation fulfill];
+    [context setShouldStop:YES];
   }];
-  [self waitForExpectationsWithTimeout:kTestTimeout handler:NULL];
+  XCTAssertTrue([[NSRunLoop currentRunLoop]
+                 gtm_runUpToSixtySecondsWithContext:context]);
   XCTAssertNotNil(runThread);
   XCTAssertNotEqualObjects(runThread, [NSThread currentThread]);
 }
@@ -86,103 +100,151 @@ static const NSTimeInterval kTestTimeout = 5;
   XCTAssertFalse([worker isExecuting]);
   XCTAssertFalse([worker isFinished]);
 
+  // Unstarted worker can be stopped without error.
+  [worker stop];
+  XCTAssertFalse([worker isExecuting]);
+  XCTAssertTrue([worker isFinished]);
 
-  // Unstarted worker can be cancelled without error.
-  [worker cancel];
+  // And can be stopped again
+  [worker stop];
   XCTAssertFalse([worker isExecuting]);
-  XCTAssertFalse([worker isFinished]);
+  XCTAssertTrue([worker isFinished]);
 
-  // And can be cancelled again
-  [worker cancel];
+  // A thread we start can be stopped with correct state.
+  worker = [[GTMSimpleWorkerThread alloc] init];
   XCTAssertFalse([worker isExecuting]);
   XCTAssertFalse([worker isFinished]);
-  [worker release];
+  [worker start];
+  XCTAssertTrue([worker isExecuting]);
+  XCTAssertFalse([worker isFinished]);
+  [worker stop];
+  XCTAssertFalse([worker isExecuting]);
+  XCTAssertTrue([worker isFinished]);
 
-  // A thread we start can be cancelled with correct state.
+  // A cancel is also honored
   worker = [[GTMSimpleWorkerThread alloc] init];
   XCTAssertFalse([worker isExecuting]);
   XCTAssertFalse([worker isFinished]);
-  XCTestExpectation *blockPerformed = [self expectationWithDescription:@"BlockIsRunning"];
   [worker start];
-  [workerThread_ gtm_performWaitingUntilDone:YES block:^{
-    [blockPerformed fulfill];
-  }];
-  [self waitForExpectationsWithTimeout:kTestTimeout handler:NULL];
   XCTAssertTrue([worker isExecuting]);
-  XCTAssertFalse([worker isCancelled]);
   XCTAssertFalse([worker isFinished]);
-  NSPredicate *predicate =
-      [NSPredicate predicateWithBlock:^BOOL(id workerThread, NSDictionary<NSString *,id> *options) {
-    return (BOOL)(![workerThread isExecuting]);
-  }];
-  [self expectationForPredicate:predicate evaluatedWithObject:worker handler:NULL];
-
   [worker cancel];
-  [self waitForExpectationsWithTimeout:kTestTimeout handler:NULL];
+  // And after some time we're done. We're generous here, this needs to
+  // exceed the worker thread's runloop timeout.
+  sleep(5);
   XCTAssertFalse([worker isExecuting]);
-  XCTAssertTrue([worker isCancelled]);
   XCTAssertTrue([worker isFinished]);
-  [worker release];
+}
+
+- (void)testWorkerThreadStopTiming {
+  // Throw a sleep and make sure that we stop as soon as we can.
+  NSDate *start = [NSDate date];
+  NSConditionLock *threadLock = [[[NSConditionLock alloc] initWithCondition:0]
+                                    autorelease];
+  [workerThread_ gtm_performBlock:^{
+    [threadLock lock];
+    [threadLock unlockWithCondition:1];
+    [NSThread sleepForTimeInterval:.25];
+  }];
+  [threadLock lockWhenCondition:1];
+  [threadLock unlock];
+  [workerThread_ stop];
+  XCTAssertFalse([workerThread_ isExecuting]);
+  XCTAssertTrue([workerThread_ isFinished]);
+  XCTAssertEqualWithAccuracy(-[start timeIntervalSinceNow], 0.25, 0.25);
 }
 
 - (void)testPerformBlockOnWorkerThread {
+  GTMUnitTestingBooleanRunLoopContext *context =
+      [GTMUnitTestingBooleanRunLoopContext context];
   __block NSThread *runThread = nil;
 
   // Runs on the other thread
-  XCTestExpectation *expectation = [self expectationWithDescription:@"BlockRan"];
+  runThread = nil;
+  [context setShouldStop:NO];
   [workerThread_ gtm_performBlock:^{
     runThread = [NSThread currentThread];
-    [expectation fulfill];
+    [context setShouldStop:YES];
   }];
-  [self waitForExpectationsWithTimeout:kTestTimeout handler:NULL];
+  XCTAssertTrue([[NSRunLoop currentRunLoop]
+                 gtm_runUpToSixtySecondsWithContext:context]);
   XCTAssertNotNil(runThread);
   XCTAssertEqualObjects(runThread, workerThread_);
 
   // Other thread no wait.
   runThread = nil;
-  expectation = [self expectationWithDescription:@"BlockRan2"];
+  [context setShouldStop:NO];
   [workerThread_ gtm_performWaitingUntilDone:NO block:^{
     runThread = [NSThread currentThread];
-    [expectation fulfill];
+    [context setShouldStop:YES];
   }];
-  [self waitForExpectationsWithTimeout:kTestTimeout handler:NULL];
+  XCTAssertTrue([[NSRunLoop currentRunLoop]
+                 gtm_runUpToSixtySecondsWithContext:context]);
   XCTAssertNotNil(runThread);
   XCTAssertEqualObjects(runThread, workerThread_);
 
   // Waiting requires no runloop spin
   runThread = nil;
+  [context setShouldStop:NO];
   [workerThread_ gtm_performWaitingUntilDone:YES block:^{
     runThread = [NSThread currentThread];
+    [context setShouldStop:YES];
   }];
+  XCTAssertTrue([context shouldStop]);
   XCTAssertNotNil(runThread);
   XCTAssertEqualObjects(runThread, workerThread_);
 }
 
-- (void)testExitingBlock {
+- (void)testExitingBlockIsExecuting {
+  NSConditionLock *threadLock = [[[NSConditionLock alloc] initWithCondition:0]
+                                    autorelease];
   [workerThread_ gtm_performWaitingUntilDone:NO block:^{
-     pthread_exit(NULL);
+    [threadLock lock];
+    [threadLock unlockWithCondition:1];
+    pthread_exit(NULL);
   }];
-  NSPredicate *predicate =
-      [NSPredicate predicateWithBlock:^BOOL(id workerThread, NSDictionary<NSString *,id> *options) {
-    return (BOOL)(![workerThread isExecuting]);
-  }];
-  [self expectationForPredicate:predicate evaluatedWithObject:workerThread_ handler:NULL];
-  [self waitForExpectationsWithTimeout:kTestTimeout handler:NULL];
+  [threadLock lockWhenCondition:1];
+  [threadLock unlock];
+  // Give the pthread_exit() a bit of time
+  [NSThread sleepForTimeInterval:.25];
+  // Did we notice the thread died? Does [... isExecuting] clean up?
+  XCTAssertFalse([workerThread_ isExecuting]);
   XCTAssertTrue([workerThread_ isFinished]);
 }
 
-
-
-- (void)testCancelFromThread {
+- (void)testExitingBlockCancel {
+  NSConditionLock *threadLock = [[[NSConditionLock alloc] initWithCondition:0]
+                                    autorelease];
   [workerThread_ gtm_performWaitingUntilDone:NO block:^{
-    [workerThread_ cancel];
+    [threadLock lock];
+    [threadLock unlockWithCondition:1];
+    pthread_exit(NULL);
   }];
-  NSPredicate *predicate =
-      [NSPredicate predicateWithBlock:^BOOL(id workerThread, NSDictionary<NSString *,id> *options) {
-    return (BOOL)(![workerThread isExecuting]);
+  [threadLock lockWhenCondition:1];
+  [threadLock unlock];
+  // Give the pthread_exit() a bit of time
+  [NSThread sleepForTimeInterval:.25];
+  // Cancel/stop the thread
+  [workerThread_ stop];
+  // Did we notice the thread died? Did we clean up?
+  XCTAssertFalse([workerThread_ isExecuting]);
+  XCTAssertTrue([workerThread_ isFinished]);
+}
+
+- (void)testStopFromThread {
+  NSConditionLock *threadLock = [[[NSConditionLock alloc] initWithCondition:0]
+                                    autorelease];
+  [workerThread_ gtm_performWaitingUntilDone:NO block:^{
+    [threadLock lock];
+    [workerThread_ stop];  // Shold not block.
+    [threadLock unlockWithCondition:1];
   }];
-  [self expectationForPredicate:predicate evaluatedWithObject:workerThread_ handler:NULL];
-  [self waitForExpectationsWithTimeout:kTestTimeout handler:NULL];
+  // Block should complete before the stop occurs.
+  [threadLock lockWhenCondition:1];
+  [threadLock unlock];
+  // Still need to give the thread a moment to not be executing
+  sleep(1);
+  XCTAssertFalse([workerThread_ isExecuting]);
   XCTAssertTrue([workerThread_ isFinished]);
 }