From 675b59127d0ec5ca7ae54647d90879b245f2d5c7 Mon Sep 17 00:00:00 2001 From: "gtm.daemon" Date: Thu, 3 Feb 2011 18:30:16 +0000 Subject: [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 --- Foundation/GTMFileSystemKQueue.m | 72 +++++++++------ Foundation/GTMFileSystemKQueueTest.m | 168 ++++++++++++++++++++++++----------- 2 files changed, 162 insertions(+), 78 deletions(-) (limited to 'Foundation') diff --git a/Foundation/GTMFileSystemKQueue.m b/Foundation/GTMFileSystemKQueue.m index e423de2..7114304 100644 --- a/Foundation/GTMFileSystemKQueue.m +++ b/Foundation/GTMFileSystemKQueue.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 @@ -68,14 +68,14 @@ static CFSocketRef gRunLoopSocket = NULL; [self release]; return nil; } - + // Make sure it imples what we expect GTMAssertSelectorNilOrImplementedWithArguments(target_, action_, @encode(GTMFileSystemKQueue*), @encode(GTMFileSystemKQueueEvents), NULL); - + fd_ = [self registerWithKQueue]; if (fd_ < 0) { [self release]; @@ -88,7 +88,7 @@ static CFSocketRef gRunLoopSocket = NULL; #if GTM_SUPPORT_GC - (void)finalize { [self unregisterWithKQueue]; - + [super finalize]; } #endif @@ -96,7 +96,7 @@ static CFSocketRef gRunLoopSocket = NULL; - (void)dealloc { [self unregisterWithKQueue]; [path_ release]; - + [super dealloc]; } @@ -113,15 +113,33 @@ static void SocketCallBack(CFSocketRef socketref, CFSocketCallBackType type, // autoreleased objects would never go away, so we provide our own pool here. NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init]; - struct kevent event; - - if (kevent(gFileSystemKQueueFileDescriptor, NULL, 0, &event, 1, NULL) == -1) { - _GTMDevLog(@"could not pick up kqueue event. Errno %d", errno); // COV_NF_LINE - } else { - GTMFileSystemKQueue *fskq = GTM_STATIC_CAST(GTMFileSystemKQueue, - event.udata); - [fskq notify:event.fflags]; - } + // We want to read as many events as possible so loop on the kevent call + // till the kqueue is empty. + int events = -1; + do { + // We wouldn't be here if CFSocket didn't think there was data on + // |gFileSystemKQueueFileDescriptor|. However, since this callback is tied + // to the runloop, if [... unregisterWithKQueue] was called before a runloop + // spin we may now be looking at an empty queue (remember, + // |gFileSystemKQueueFileDescriptor| isn't a normal descriptor). + + // Try to consume one event with an immediate timeout. + struct kevent event; + const struct timespec noWait = { 0, 0 }; + events = kevent(gFileSystemKQueueFileDescriptor, NULL, 0, &event, 1, &noWait); + + if (events == 1) { + GTMFileSystemKQueue *fskq = GTM_STATIC_CAST(GTMFileSystemKQueue, + event.udata); + [fskq notify:event.fflags]; + } else if (events == -1) { + _GTMDevLog(@"could not pick up kqueue event. Errno %d", errno); // COV_NF_LINE + } else { + // |events| is zero, either we've drained the kqueue or CFSocket was + // notified and then the events went away before we had a chance to see + // them. + } + } while (events > 0); [pool drain]; } @@ -129,7 +147,7 @@ static void SocketCallBack(CFSocketRef socketref, CFSocketCallBackType type, // Cribbed from Advanced Mac OS X Programming - (void)addFileDescriptorMonitor:(int)fd { _GTMDevAssert(gRunLoopSocket == NULL, @"socket should be NULL at this point"); - + CFSocketContext context = { 0, NULL, NULL, NULL, NULL }; gRunLoopSocket = CFSocketCreateWithNative(kCFAllocatorDefault, fd, @@ -140,26 +158,26 @@ static void SocketCallBack(CFSocketRef socketref, CFSocketCallBackType type, _GTMDevLog(@"could not CFSocketCreateWithNative"); // COV_NF_LINE goto bailout; // COV_NF_LINE } - + CFRunLoopSourceRef rls; rls = CFSocketCreateRunLoopSource(NULL, gRunLoopSocket, 0); if (rls == NULL) { _GTMDevLog(@"could not create a run loop source"); // COV_NF_LINE goto bailout; // COV_NF_LINE } - + CFRunLoopAddSource(CFRunLoopGetCurrent(), rls, kCFRunLoopDefaultMode); CFRelease(rls); - + bailout: return; - + } // Returns the FD we got in registering - (int)registerWithKQueue { - + // Make sure we have our kqueue. if (gFileSystemKQueueFileDescriptor == 0) { gFileSystemKQueueFileDescriptor = kqueue(); @@ -174,15 +192,15 @@ static void SocketCallBack(CFSocketRef socketref, CFSocketCallBackType type, // Add the kqueue file descriptor to the runloop. [self addFileDescriptorMonitor:gFileSystemKQueueFileDescriptor]; } - + int newFD = open([path_ fileSystemRepresentation], O_EVTONLY, 0); if (newFD >= 0) { // Add a new event for the file. struct kevent filter; - EV_SET(&filter, newFD, EVFILT_VNODE, EV_ADD | EV_ENABLE | EV_CLEAR, + EV_SET(&filter, newFD, EVFILT_VNODE, EV_ADD | EV_ENABLE | EV_CLEAR, events_, 0, self); - + const struct timespec noWait = { 0, 0 }; if (kevent(gFileSystemKQueueFileDescriptor, &filter, 1, NULL, 0, &noWait) == -1) { // COV_NF_START @@ -192,7 +210,7 @@ static void SocketCallBack(CFSocketRef socketref, CFSocketCallBackType type, // COV_NF_END } } - + return newFD; } @@ -217,14 +235,14 @@ static void SocketCallBack(CFSocketRef socketref, CFSocketCallBackType type, - (void)notify:(GTMFileSystemKQueueEvents)eventFFlags { // Some notifications get a little bit of overhead first - + if (eventFFlags & NOTE_REVOKE) { // COV_NF_START - no good way to do this in a unittest // Assume revoke means unmount and give up [self unregisterWithKQueue]; // COV_NF_END } - + if (eventFFlags & NOTE_DELETE) { [self unregisterWithKQueue]; if (acrossReplace_) { 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 -- cgit v1.2.3