From 8a5be1dcdf93aed6d6b6f8e676ca218157b26283 Mon Sep 17 00:00:00 2001 From: dmaclach Date: Mon, 12 Nov 2018 08:02:02 -0800 Subject: Fixes up a race condition in GTMNSThread+Blocks (#181) There was a race between the thread being finished and isFinished/isExecuting reporting correctly. There may have also been a locking issue on older single processor phones. --- Foundation/GTMNSThread+Blocks.m | 240 +++++++--------------------------------- 1 file changed, 41 insertions(+), 199 deletions(-) (limited to 'Foundation/GTMNSThread+Blocks.m') diff --git a/Foundation/GTMNSThread+Blocks.m b/Foundation/GTMNSThread+Blocks.m index 8318193..3ef4a45 100644 --- a/Foundation/GTMNSThread+Blocks.m +++ b/Foundation/GTMNSThread+Blocks.m @@ -53,220 +53,62 @@ #endif // NS_BLOCKS_AVAILABLE -enum { - kGTMSimpleThreadInitialized = 0, - kGTMSimpleThreadStarting, - kGTMSimpleThreadRunning, - kGTMSimpleThreadCancel, - kGTMSimpleThreadFinished, -}; - @implementation GTMSimpleWorkerThread -- (id)init { - if ((self = [super init])) { - runLock_ = - [[NSConditionLock alloc] initWithCondition:kGTMSimpleThreadInitialized]; - } - return self; -} - -- (void)dealloc { - if ([self isExecuting]) { - [self stop]; - } - [runLock_ release]; - [super dealloc]; -} - -- (void)setThreadDebuggerName:(NSString *)name { - if ([name length]) { - pthread_setname_np([name UTF8String]); - } else { - pthread_setname_np(""); - } -} - - (void)main { - [runLock_ lock]; - if ([runLock_ condition] != kGTMSimpleThreadStarting) { - // Don't start, we're already cancelled or we've been started twice. - [runLock_ unlock]; - return; - } - - // 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]; - } - - // Exit - [runLock_ lock]; - [localPool drain]; - if (runLoop_) CFRelease(runLoop_); - runLoop_ = NULL; - [runLock_ unlockWithCondition:kGTMSimpleThreadFinished]; -} - -- (void)start { - // 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; + NSRunLoop *nsRunLoop = [NSRunLoop currentRunLoop]; + // According to the NSRunLoop docs, a port must be added to the + // runloop to keep the loop alive, otherwise when you call + // runMode:beforeDate: it will immediately return with NO. We never + // send anything over this port, it's only here to keep the run loop + // looping. + [nsRunLoop addPort:[NSPort port] forMode:NSDefaultRunLoopMode]; + while (true) { + if (self.isCancelled) { + break; + } + BOOL ranLoop = [nsRunLoop runMode:NSDefaultRunLoopMode + beforeDate:[NSDate distantFuture]]; + if (!ranLoop) { + break; + } } - [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]; - [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; + if (![[NSThread currentThread] isEqual:self]) { + // This call just forces the runloop in main to spin allowing main to see + // that the isCancelled flag has been set. Note that this is only really + // needed if there are no blocks/selectors in the queue for the thread. If + // there are other items to be processed in the queue, the next one will be + // executed and then the "cancel" will be seen in main, and it will exit + // (and the other blocks will be dropped). + [self performSelector:@selector(class) + onThread:self + withObject:nil + waitUntilDone:NO]; } } - (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:) + if ([[NSThread currentThread] isEqual:self]) { + [super cancel]; + } else { + // This call forces the runloop in main to spin allowing main to see that + // the isCancelled flag has been set. Note that we explicitly want to send + // it to the thread to process so it is added to the end of the queue of + // blocks to be processed. 'stop' guarantees that all items in the queue + // will be processed before it ends. + [self performSelector:@selector(cancel) onThread:self - withObject:name + withObject:nil waitUntilDone:YES]; + while (![self isFinished] || [self isExecuting]) { + // Spin until the thread is really done. + usleep(10); + } } - [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 -- cgit v1.2.3