[Author: aharper]

Fix interaction of GTMFileSystemKqueue when the runloop isn't spinning. Also,
don't count on kCFSocketReadCallBack to keep calling us, empty the queue
whenever we can.

DELTA=93 (84 added, 0 deleted, 9 changed)
R=dmaclach,thomasvl

1 files changed, 117 insertions, 51 deletions

diff --git a/Foundation/GTMFileSystemKQueueTest.m b/Foundation/GTMFileSystemKQueueTest.m
index a64fd2b..d80f161 100644
--- a/Foundation/GTMFileSystemKQueueTest.m
+++ b/Foundation/GTMFileSystemKQueueTest.m
@@ -6,9 +6,9 @@
 //  Licensed under the Apache License, Version 2.0 (the "License"); you may not
 //  use this file except in compliance with the License.  You may obtain a copy
 //  of the License at
-// 
+//
 //  http://www.apache.org/licenses/LICENSE-2.0
-// 
+//
 //  Unless required by applicable law or agreed to in writing, software
 //  distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
 //  WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
@@ -18,8 +18,16 @@
 
 #import "GTMSenTestCase.h"
 #import "GTMFileSystemKQueue.h"
+#import "GTMGarbageCollection.h"
 #import "GTMUnitTestDevLog.h"
 
+
+// Private methods of GTMFileSystemKQueue we use for some tests
+@interface GTMFileSystemKQueue (PrivateMethods)
+- (void)unregisterWithKQueue;
+@end
+
+
 @interface GTMFileSystemKQueueTest : GTMTestCase {
  @private
   NSString *testPath_;
@@ -38,13 +46,13 @@
 
 @implementation GTMFSKQTestHelper
 
-- (void)callbackForQueue:(GTMFileSystemKQueue *)queue 
+- (void)callbackForQueue:(GTMFileSystemKQueue *)queue
                   events:(GTMFileSystemKQueueEvents)event {
   // Can't use standard ST macros here because our helper
   // is not a subclass of GTMTestCase. This is intentional.
   if (queue != queue_) {
     NSString *file = [NSString stringWithUTF8String:__FILE__];
-    NSException *exception 
+    NSException *exception
       = [NSException failureInEqualityBetweenObject:queue
                                           andObject:queue_
                                              inFile:file
@@ -52,7 +60,7 @@
                                     withDescription:nil];
     [exception raise];
   }
-  
+
   if (event & kGTMFileSystemKQueueWriteEvent) {
     ++writes_;
   }
@@ -115,7 +123,7 @@
   [fm removeFileAtPath:testPath_ handler:nil];
   [fm removeFileAtPath:testPath2_ handler:nil];
 #endif  // MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
-  
+
   [testPath_ release];
   testPath_ = nil;
   [testPath2_ release];
@@ -126,13 +134,13 @@
   GTMFileSystemKQueue *testKQ;
   GTMFSKQTestHelper *helper = [[[GTMFSKQTestHelper alloc] init] autorelease];
   STAssertNotNil(helper, nil);
-  
+
   // init should fail
   [GTMUnitTestDevLog expectString:@"Don't call init, use "
                      @"initWithPath:forEvents:acrossReplace:target:action:"];
   testKQ = [[[GTMFileSystemKQueue alloc] init] autorelease];
   STAssertNil(testKQ, nil);
-  
+
   // no path
   testKQ
     = [[[GTMFileSystemKQueue alloc] initWithPath:nil
@@ -141,7 +149,7 @@
                                           target:helper
                                           action:@selector(callbackForQueue:events:)] autorelease];
   STAssertNil(testKQ, nil);
-  
+
   // not events
   testKQ
     = [[[GTMFileSystemKQueue alloc] initWithPath:@"/var/log/system.log"
@@ -150,7 +158,7 @@
                                           target:helper
                                           action:@selector(callbackForQueue:events:)] autorelease];
   STAssertNil(testKQ, nil);
-  
+
   // no target
   testKQ
     = [[[GTMFileSystemKQueue alloc] initWithPath:@"/var/log/system.log"
@@ -159,7 +167,7 @@
                                           target:nil
                                           action:@selector(callbackForQueue:events:)] autorelease];
   STAssertNil(testKQ, nil);
-  
+
   // no handler
   testKQ
     = [[[GTMFileSystemKQueue alloc] initWithPath:@"/var/log/system.log"
@@ -169,7 +177,7 @@
                                           action:nil] autorelease];
   STAssertNil(testKQ, nil);
 
-  
+
   // path that doesn't exist
   testKQ
     = [[[GTMFileSystemKQueue alloc] initWithPath:@"/path/that/does/not/exist"
@@ -191,15 +199,15 @@
 }
 
 - (void)testWriteAndDelete {
-  
+
   NSFileManager *fm = [NSFileManager defaultManager];
   GTMFSKQTestHelper *helper = [[[GTMFSKQTestHelper alloc] init] autorelease];
   STAssertNotNil(helper, nil);
-  
+
   STAssertTrue([fm createFileAtPath:testPath_ contents:nil attributes:nil], nil);
   NSFileHandle *testFH = [NSFileHandle fileHandleForWritingAtPath:testPath_];
   STAssertNotNil(testFH, nil);
-  
+
   // Start monitoring the file
   GTMFileSystemKQueue *testKQ
     = [[GTMFileSystemKQueue alloc] initWithPath:testPath_
@@ -210,14 +218,14 @@
   STAssertNotNil(testKQ, nil);
   STAssertEqualObjects([testKQ path], testPath_, nil);
   [helper setKQueue:testKQ];
-  
+
   // Write to the file
   [testFH writeData:[@"doh!" dataUsingEncoding:NSUnicodeStringEncoding]];
-  
+
   // Spin the runloop for a second so that the helper callbacks fire
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 1, nil);
-  
+
   // Close and delete
   [testFH closeFile];
 #if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
@@ -226,34 +234,34 @@
 #else
   STAssertTrue([fm removeFileAtPath:testPath_ handler:nil], nil);
 #endif  // MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
-  
+
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 2, nil);
 
   // Clean up the kqueue
   [testKQ release];
   testKQ = nil;
-  
+
   STAssertEquals([helper writes], 1, nil);
   STAssertEquals([helper deletes], 1, nil);
   STAssertEquals([helper renames], 0, nil);
 }
 
 - (void)testWriteAndDeleteAndWrite {
-  
+
   // One will pass YES to |acrossReplace|, the other will pass NO.
-  
+
   NSFileManager *fm = [NSFileManager defaultManager];
   GTMFSKQTestHelper *helper = [[[GTMFSKQTestHelper alloc] init] autorelease];
   STAssertNotNil(helper, nil);
   GTMFSKQTestHelper *helper2 = [[[GTMFSKQTestHelper alloc] init] autorelease];
   STAssertNotNil(helper, nil);
-  
+
   // Create a temp file path
   STAssertTrue([fm createFileAtPath:testPath_ contents:nil attributes:nil], nil);
   NSFileHandle *testFH = [NSFileHandle fileHandleForWritingAtPath:testPath_];
   STAssertNotNil(testFH, nil);
-  
+
   // Start monitoring the file
   GTMFileSystemKQueue *testKQ
     = [[GTMFileSystemKQueue alloc] initWithPath:testPath_
@@ -264,7 +272,7 @@
   STAssertNotNil(testKQ, nil);
   STAssertEqualObjects([testKQ path], testPath_, nil);
   [helper setKQueue:testKQ];
-  
+
   GTMFileSystemKQueue *testKQ2
     = [[GTMFileSystemKQueue alloc] initWithPath:testPath_
                                       forEvents:kGTMFileSystemKQueueAllEvents
@@ -274,7 +282,7 @@
   STAssertNotNil(testKQ2, nil);
   STAssertEqualObjects([testKQ2 path], testPath_, nil);
   [helper2 setKQueue:testKQ2];
-  
+
   // Write to the file
   [testFH writeData:[@"doh!" dataUsingEncoding:NSUnicodeStringEncoding]];
 
@@ -297,7 +305,7 @@
   testFH = [NSFileHandle fileHandleForWritingAtPath:testPath_];
   STAssertNotNil(testFH, nil);
   [testFH writeData:[@"ha!" dataUsingEncoding:NSUnicodeStringEncoding]];
-  
+
   // Spin the runloop for a second so that the helper callbacks fire
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 2, nil);
@@ -305,12 +313,12 @@
 
   // Write to it again
   [testFH writeData:[@"continued..." dataUsingEncoding:NSUnicodeStringEncoding]];
-  
+
   // Spin the runloop for a second so that the helper callbacks fire
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 3, nil);
   STAssertEquals([helper2 totals], 2, nil);
-  
+
   // Close and delete
   [testFH closeFile];
 #if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
@@ -318,18 +326,18 @@
 #else
   STAssertTrue([fm removeFileAtPath:testPath_ handler:nil], nil);
 #endif  // MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
-  
+
   // Spin the runloop for a second so that the helper callbacks fire
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 4, nil);
   STAssertEquals([helper2 totals], 2, nil);
-  
+
   // Clean up the kqueue
   [testKQ release];
   testKQ = nil;
   [testKQ2 release];
   testKQ2 = nil;
-  
+
   STAssertEquals([helper writes], 2, nil);
   STAssertEquals([helper deletes], 2, nil);
   STAssertEquals([helper renames], 0, nil);
@@ -339,7 +347,7 @@
 }
 
 - (void)testWriteAndRenameAndWrite {
-  
+
   // One will pass YES to |acrossReplace|, the other will pass NO.
 
   NSFileManager *fm = [NSFileManager defaultManager];
@@ -347,12 +355,12 @@
   STAssertNotNil(helper, nil);
   GTMFSKQTestHelper *helper2 = [[[GTMFSKQTestHelper alloc] init] autorelease];
   STAssertNotNil(helper2, nil);
-  
+
   // Create a temp file path
   STAssertTrue([fm createFileAtPath:testPath_ contents:nil attributes:nil], nil);
   NSFileHandle *testFH = [NSFileHandle fileHandleForWritingAtPath:testPath_];
   STAssertNotNil(testFH, nil);
-  
+
   // Start monitoring the file
   GTMFileSystemKQueue *testKQ
     = [[GTMFileSystemKQueue alloc] initWithPath:testPath_
@@ -363,7 +371,7 @@
   STAssertNotNil(testKQ, nil);
   STAssertEqualObjects([testKQ path], testPath_, nil);
   [helper setKQueue:testKQ];
-  
+
   GTMFileSystemKQueue *testKQ2
     = [[GTMFileSystemKQueue alloc] initWithPath:testPath_
                                       forEvents:kGTMFileSystemKQueueAllEvents
@@ -373,43 +381,43 @@
   STAssertNotNil(testKQ2, nil);
   STAssertEqualObjects([testKQ2 path], testPath_, nil);
   [helper2 setKQueue:testKQ2];
-  
+
   // Write to the file
   [testFH writeData:[@"doh!" dataUsingEncoding:NSUnicodeStringEncoding]];
-  
+
   // Spin the runloop for a second so that the helper callbacks fire
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 1, nil);
   STAssertEquals([helper2 totals], 1, nil);
-  
+
   // Move it and create the file again
 #if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
   NSError *error = nil;
-  STAssertTrue([fm moveItemAtPath:testPath_ toPath:testPath2_ error:&error], 
+  STAssertTrue([fm moveItemAtPath:testPath_ toPath:testPath2_ error:&error],
                @"Error: %@", error);
 #else
   STAssertTrue([fm movePath:testPath_ toPath:testPath2_ handler:nil], nil);
 #endif  // MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
-  
+
   STAssertTrue([fm createFileAtPath:testPath_ contents:nil attributes:nil], nil);
   NSFileHandle *testFHPrime
     = [NSFileHandle fileHandleForWritingAtPath:testPath_];
   STAssertNotNil(testFHPrime, nil);
   [testFHPrime writeData:[@"eh?" dataUsingEncoding:NSUnicodeStringEncoding]];
-  
+
   // Spin the runloop for a second so that the helper callbacks fire
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 2, nil);
   STAssertEquals([helper2 totals], 2, nil);
-  
+
   // Write to the new file
   [testFHPrime writeData:[@"continue..." dataUsingEncoding:NSUnicodeStringEncoding]];
-  
+
   // Spin the runloop for a second so that the helper callbacks fire
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 3, nil);
   STAssertEquals([helper2 totals], 2, nil);
-  
+
   // Write to the old file
   [testFH writeData:[@"continue old..." dataUsingEncoding:NSUnicodeStringEncoding]];
 
@@ -417,7 +425,7 @@
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 3, nil);
   STAssertEquals([helper2 totals], 3, nil);
-  
+
   // and now close old
   [testFH closeFile];
 #if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
@@ -425,12 +433,12 @@
 #else
   STAssertTrue([fm removeFileAtPath:testPath2_ handler:nil], nil);
 #endif  // MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
-  
+
   // Spin the runloop for a second so that the helper callbacks fire
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 3, nil);
   STAssertEquals([helper2 totals], 4, nil);
-  
+
   // and now close new
   [testFHPrime closeFile];
 #if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
@@ -438,18 +446,18 @@
 #else
   STAssertTrue([fm removeFileAtPath:testPath_ handler:nil], nil);
 #endif  // MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
-  
+
   // Spin the runloop for a second so that the helper callbacks fire
   [self spinForEvents:helper];
   STAssertEquals([helper totals], 4, nil);
   STAssertEquals([helper2 totals], 4, nil);
-  
+
   // Clean up the kqueue
   [testKQ release];
   testKQ = nil;
   [testKQ2 release];
   testKQ2 = nil;
-  
+
   STAssertEquals([helper writes], 2, nil);
   STAssertEquals([helper deletes], 1, nil);
   STAssertEquals([helper renames], 1, nil);
@@ -458,4 +466,62 @@
   STAssertEquals([helper2 renames], 1, nil);
 }
 
+- (void)testNoSpinHang {
+  // This case tests a specific historically problematic interaction of
+  // GTMFileSystemKQueue and the runloop. GTMFileSystemKQueue uses the CFSocket
+  // notifications (and thus the runloop) for monitoring, however, you can
+  // dealloc the instance (and thus unregister the underlying kevent descriptor)
+  // prior to any runloop spin. The unregister removes the pending notifications
+  // from the monitored main kqueue file descriptor that CFSocket has previously
+  // noticed but not yet called back. At that point a kevent() call in the
+  // socket callback without a timeout would hang the runloop.
+
+  // Warn this may hang
+  NSLog(@"%s on failure this will hang.", __PRETTY_FUNCTION__);
+
+  NSFileManager *fm = [NSFileManager defaultManager];
+  GTMFSKQTestHelper *helper = [[[GTMFSKQTestHelper alloc] init] autorelease];
+  STAssertNotNil(helper, nil);
+  STAssertTrue([fm createFileAtPath:testPath_ contents:nil attributes:nil], nil);
+  NSFileHandle *testFH = [NSFileHandle fileHandleForWritingAtPath:testPath_];
+  STAssertNotNil(testFH, nil);
+
+  // Start monitoring the file
+  GTMFileSystemKQueue *testKQ
+    = [[GTMFileSystemKQueue alloc] initWithPath:testPath_
+                                      forEvents:kGTMFileSystemKQueueAllEvents
+                                  acrossReplace:YES
+                                         target:helper
+                                         action:@selector(callbackForQueue:events:)];
+  STAssertNotNil(testKQ, nil);
+  STAssertEqualObjects([testKQ path], testPath_, nil);
+  [helper setKQueue:testKQ];
+
+  // Write to the file
+  [testFH writeData:[@"doh!" dataUsingEncoding:NSUnicodeStringEncoding]];
+  // Close and delete
+  [testFH closeFile];
+#if MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
+  NSError *error = nil;
+  STAssertTrue([fm removeItemAtPath:testPath_ error:&error], @"Err: %@", error);
+#else
+  STAssertTrue([fm removeFileAtPath:testPath_ handler:nil], nil);
+#endif  // MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_X_VERSION_10_5
+
+  // Now destroy the queue, with events outstanding from the CFSocket, but
+  // unconsumed. For GC this involves us using a private method since |helper|
+  // still has a reference. For non-GC we'll force the release.
+  if (GTMIsGarbageCollectionEnabled()) {
+    [testKQ unregisterWithKQueue];
+  } else {
+    STAssertEquals([testKQ retainCount], (NSUInteger)1, nil);
+    [testKQ release];
+    testKQ = nil;
+  }
+
+  // Spin the runloop, no events were delivered (and we should not hang)
+  [self spinForEvents:helper];
+  STAssertEquals([helper totals], 0, nil);
+}
+
 @end