Menu
▾
▴
[Bibdesk-commit] SF.net SVN: bibdesk:[18635] trunk/bibdesk/BDSKTask.m
|
From: <ama...@us...> - 2012-05-18 16:28:31
|
Revision: 18635
http://bibdesk.svn.sourceforge.net/bibdesk/?rev=18635&view=rev
Author: amaxwell
Date: 2012-05-18 16:28:24 +0000 (Fri, 18 May 2012)
Log Message:
-----------
BDSKTask fixes from TeX Live Utility codebase. From my logs, changes
appear to be the following:
- Autorelease to shut the static analyzer up
- Log if waitpid fails
- Implement -finalize for GC support.
Move code that frees the internal guts to _disableNotification
- Copy args and environment with strdup to avoid garbage collection problems
with -[NSString fileSystemRepresentation].
- From looking at Jason Harris' crash log, he was entirely correct about an
empty string causing an assertion failure and crash in BDSKTask with an
empty string as argument. Replace -[NSString fileSystemRepresentation]
usage with CoreFoundation to avoid that braindead behavior.
- Use -drain for autorelease pool
- Add a macro from chromium to test for EINTR on some syscalls
- Implement -terminationReason
- More complete logging when fork fails
- Retain task earlier in kevent handler, which may shorten a very small
race window that I hit in another project
Modified Paths:
--------------
trunk/bibdesk/BDSKTask.m
Modified: trunk/bibdesk/BDSKTask.m
===================================================================
--- trunk/bibdesk/BDSKTask.m 2012-05-18 06:35:36 UTC (rev 18634)
+++ trunk/bibdesk/BDSKTask.m 2012-05-18 16:28:24 UTC (rev 18635)
@@ -56,11 +56,20 @@
@end
+#ifndef MAC_OS_X_VERSION_10_6
+enum {
+ NSTaskTerminationReasonExit = 1,
+ NSTaskTerminationReasonUncaughtSignal = 2
+};
+typedef NSInteger NSTaskTerminationReason;
+#endif
+
struct BDSKTaskInternal {
int32_t _terminationStatus;
+ NSTaskTerminationReason _terminationReason;
int32_t _running;
int32_t _launched;
- int32_t _canNotify;
+ volatile int32_t _canNotify;
struct kevent _event;
CFRunLoopRef _rl;
CFRunLoopSourceRef _rlsource;
@@ -105,17 +114,6 @@
- (void)dealloc
{
- /*
- Set _canNotify in case kevent unblocks before we can remove it from the queue,
- since the event's task pointer is about to become invalid (and it mustn't access
- the lock after this flag is set). Lock before entering _disableNotification, so
- we can shrink our race window even smaller.
- */
- OSAtomicCompareAndSwap32Barrier(1, 0, &_internal->_canNotify);
- pthread_mutex_lock(&_internal->_lock);
- [self _disableNotification];
- pthread_mutex_unlock(&_internal->_lock);
- pthread_mutex_destroy(&_internal->_lock);
BDSKDESTROY(_launchPath);
BDSKDESTROY(_arguments);
BDSKDESTROY(_environment);
@@ -123,13 +121,15 @@
BDSKDESTROY(_standardInput);
BDSKDESTROY(_standardOutput);
BDSKDESTROY(_standardError);
- // runloop and source are freed in __BDSKTaskNotify or _disableNotification
- NSParameterAssert(NULL == _internal->_rl);
- NSParameterAssert(NULL == _internal->_rlsource);
- BDSKZONEDESTROY(_internal);
[super dealloc];
}
+- (void)finalize
+{
+ [self _disableNotification];
+ [super finalize];
+}
+
- (void)setLaunchPath:(NSString *)path;
{
ASSERT_NOTLAUNCHED;
@@ -215,17 +215,44 @@
[task release];
}
+/*
+ Undocumented behavior of -[NSFileManager fileSystemRepresentationWithPath:]
+ is to raise an exception when passed an empty string. Since this is called by
+ -[NSString fileSystemRepresentation], use CF. rdar://problem/9565599
+
+ https://bitbucket.org/jfh/machg/issue/244/p1d3-crash-during-view-differences
+
+ Have to copy all -[NSString fileSystemRepresentation] pointers to avoid garbage collection
+ issues with -fileSystemRepresentation, anyway. How tedious compared to -autorelease...
+
+ http://lists.apple.com/archives/objc-language/2011/Mar/msg00122.html
+ */
+static char *__BDSKCopyFileSystemRepresentation(NSString *str)
+{
+ if (nil == str) return NULL;
+
+ CFIndex len = CFStringGetMaximumSizeOfFileSystemRepresentation((CFStringRef)str);
+ char *cstr = NSZoneCalloc(NSDefaultMallocZone(), len, sizeof(char));
+ if (CFStringGetFileSystemRepresentation((CFStringRef)str, cstr, len) == FALSE) {
+ NSZoneFree(NSDefaultMallocZone(), cstr);
+ cstr = NULL;
+ }
+ return cstr;
+}
+
- (void)launch;
{
ASSERT_NOTLAUNCHED;
- NSUInteger argCount = [_arguments count];
- const char *workingDir = [_currentDirectoryPath fileSystemRepresentation];
+ const NSUInteger argCount = [_arguments count];
+ char *workingDir = __BDSKCopyFileSystemRepresentation(_currentDirectoryPath);
+
+ // fill with pointers to copied C strings
char **args = NSZoneCalloc([self zone], (argCount + 2), sizeof(char *));
- NSUInteger i, iMax = argCount;
- args[0] = (char *)[_launchPath fileSystemRepresentation];
- for (i = 0; i < iMax; i++) {
- args[i + 1] = (char *)[[_arguments objectAtIndex:i] fileSystemRepresentation];
+ NSUInteger i;
+ args[0] = __BDSKCopyFileSystemRepresentation(_launchPath);
+ for (i = 0; i < argCount; i++) {
+ args[i + 1] = __BDSKCopyFileSystemRepresentation([_arguments objectAtIndex:i]);
}
args[argCount + 1] = NULL;
@@ -234,12 +261,12 @@
NSDictionary *environment = [self environment];
if (environment) {
- // fill with pointers to autoreleased C strings
+ // fill with pointers to copied C strings
env = NSZoneCalloc([self zone], [environment count] + 1, sizeof(char *));
NSString *key;
NSUInteger envIndex = 0;
for (key in environment) {
- env[envIndex++] = (char *)[[NSString stringWithFormat:@"%@=%@", key, [environment objectForKey:key]] UTF8String];
+ env[envIndex++] = __BDSKCopyFileSystemRepresentation([NSString stringWithFormat:@"%@=%@", key, [environment objectForKey:key]]);
}
env[envIndex] = NULL;
}
@@ -249,7 +276,7 @@
id fh = nil;
// the end of a pipe passed to the child needs to be closed in the parent process
- NSMutableSet *handlesToClose = [NSMutableSet new];
+ NSMutableSet *handlesToClose = [NSMutableSet set];
fh = [self standardInput];
if ([fh isKindOfClass:[NSPipe class]]) {
@@ -316,12 +343,12 @@
them. This was a very confusing race to debug, and it resulted in a bunch of orphaned child
processes.
- Using a class-scope lock is one possible solution, but NSTask doesn't use that log, and subclasses
+ Using a class-scope lock is one possible solution, but NSTask couldn't use that lock, and subclasses
that override -launch would also not benefit from locking (e.g., TLMTask). Since TLMTask sets up
NSPipes in -launch before calling -[super launch], those pipes and any created by Cocoa would not
- be protected by that lock. Closing all remaining file descriptors doesn't break any documented
- behavior of NSTask, and it should take care of that problem. It's not a great solution, since
- inheriting other descriptors could possibly be useful, but I don't need to share arbitrary file
+ be protected by that lock. Closing all remaining file descriptors in the child doesn't break any
+ documented behavior of NSTask, and it should take care of that problem. It's not a great solution,
+ since inheriting other descriptors could possibly be useful, but I don't need to share arbitrary file
descriptors, whereas I do need subclassing and threads to work properly.
*/
rlim_t j;
@@ -334,7 +361,7 @@
char ignored;
// block until the parent has setup complete
- read(blockpipe[0], &ignored, 1);
+ (void) HANDLE_EINTR(read(blockpipe[0], &ignored, 1));
close(blockpipe[0]);
int ret = execve(args[0], args, env);
@@ -342,7 +369,8 @@
}
else if (-1 == _processIdentifier) {
// parent: error
- perror("fork() failed");
+ int forkError = errno;
+ NSLog(@"fork() failed in task %@: %s", self, strerror(forkError));
_internal->_terminationStatus = 2;
}
else {
@@ -355,7 +383,7 @@
// NSTask docs say that these descriptors are closed in the parent task; required to make pipes work properly
[handlesToClose makeObjectsPerformSelector:@selector(closeFile)];
- if (-1 != fd_null) close(fd_null);
+ if (-1 != fd_null) (void) close(fd_null);
/*
The kevent will have a weak reference to this task, so -dealloc can occur without waiting for notification.
@@ -363,7 +391,7 @@
around after the exec.
*/
EV_SET(&_internal->_event, _processIdentifier, EVFILT_PROC, EV_ADD, NOTE_EXIT | NOTE_SIGNAL, 0, self);
- kevent(_kqueue, &_internal->_event, 1, NULL, 0, NULL);
+ (void) HANDLE_EINTR(kevent(_kqueue, &_internal->_event, 1, NULL, 0, NULL));
// use a runloop source to ensure that the notification is posted on the correct thread
_internal->_rl = (CFRunLoopRef)CFRetain(CFRunLoopGetCurrent());
@@ -380,11 +408,28 @@
}
// executed by child and parent
- [handlesToClose release];
+
+ /*
+ Free all the copied C strings. Don't modify the base pointer of args or env, since we have to
+ free those too!
+ */
+ free(workingDir);
+ char **freePtr = args;
+ while (NULL != *freePtr) {
+ free(*freePtr++);
+ }
+
NSZoneFree(NSZoneFromPointer(args), args);
- if (*nsEnvironment != env) NSZoneFree(NSZoneFromPointer(env), env);
+ if (*nsEnvironment != env) {
+ freePtr = env;
+ while (NULL != *freePtr) {
+ free(*freePtr++);
}
+ NSZoneFree(NSZoneFromPointer(env), env);
+ }
+}
+
- (void)interrupt;
{
ASSERT_LAUNCH;
@@ -424,6 +469,13 @@
return _internal->_terminationStatus;
}
+- (NSTaskTerminationReason)terminationReason;
+{
+ ASSERT_LAUNCH;
+ if ([self isRunning]) [NSException raise:NSInternalInconsistencyException format:@"Task is still running"];
+ return _internal->_terminationReason;
+}
+
- (void)waitUntilExit;
{
ASSERT_LAUNCH;
@@ -445,31 +497,35 @@
NSAutoreleasePool *pool = [NSAutoreleasePool new];
struct kevent evt;
- if (kevent(_kqueue, NULL, 0, &evt, 1, NULL)) {
+ if (HANDLE_EINTR(kevent(_kqueue, NULL, 0, &evt, 1, NULL))) {
- BDSKTask *task = evt.udata;
- OSMemoryBarrier();
-
- // can only fail if _disableNotification is called immediately after kevent unblocks
- if (task->_internal->_canNotify && pthread_mutex_trylock(&task->_internal->_lock) == 0) {
-
/*
Retain to make sure we hold a reference to the task long enough to handle these calls,
so we're guaranteed that _disableNotification will not be called during another callout.
*/
- task = [task retain];
- pthread_mutex_unlock(&task->_internal->_lock);
+ BDSKTask *task = [(id)evt.udata retain];
- if ((evt.fflags & NOTE_EXIT) == NOTE_EXIT)
- [task _taskExited];
- else if ((evt.fflags & NOTE_SIGNAL) == NOTE_SIGNAL)
+ // for task->_internal->_canNotify
+ OSMemoryBarrier();
+
+ // can only fail if _disableNotification is called immediately after kevent unblocks
+ if (task->_internal->_canNotify) {
+
+ if (evt.fflags & NOTE_SIGNAL)
[task _taskSignaled];
- [task release];
+ if (evt.fflags & NOTE_EXIT)
+ [task _taskExited];
+
}
+ else {
+ NSLog(@"Ignoring kqueue event for deallocated task %p", task);
+ }
+ [task release];
+
}
- [pool release];
+ [pool drain];
} while (1);
}
@@ -478,7 +534,7 @@
- (void)_taskSignaled
{
int status;
- if (waitpid(_processIdentifier, &status, WNOHANG)) {
+ if (HANDLE_EINTR(waitpid(_processIdentifier, &status, WNOHANG))) {
if (WIFSIGNALED(status))
NSLog(@"task terminated with signal %d", WTERMSIG(status));
else if (WIFSTOPPED(status))
@@ -493,12 +549,24 @@
*/
- (void)_disableNotification
{
+ // should only be called once
+ NSParameterAssert(NULL != _internal);
+
+ /*
+ Set _canNotify in case kevent unblocks before we can remove it from the queue,
+ since the event's task pointer is about to become invalid (and it mustn't access
+ the lock after this flag is set). Lock before entering _disableNotification, so
+ we can shrink our race window even smaller.
+ */
+ OSAtomicCompareAndSwap32Barrier(1, 0, &_internal->_canNotify);
+ pthread_mutex_lock(&_internal->_lock);
+
// called unconditionally from -dealloc, so we may have already notified and freed this source
if (_internal->_rlsource) {
// after this point, _taskExited and __BDSKTaskNotify will never be called, so account for their teardown
_internal->_event.flags = EV_DELETE;
- kevent(_kqueue, &_internal->_event, 1, NULL, 0, NULL);
+ (void) HANDLE_EINTR(kevent(_kqueue, &_internal->_event, 1, NULL, 0, NULL));
CFRunLoopSourceInvalidate(_internal->_rlsource);
_internal->_rlsource = NULL;
@@ -510,6 +578,15 @@
_internal->_rl = NULL;
}
}
+
+ pthread_mutex_unlock(&_internal->_lock);
+ pthread_mutex_destroy(&_internal->_lock);
+
+ // runloop and source are freed in __BDSKTaskNotify or _disableNotification
+ NSParameterAssert(NULL == _internal->_rl);
+ NSParameterAssert(NULL == _internal->_rlsource);
+ NSZoneFree(NSZoneFromPointer(_internal), _internal);
+ _internal = NULL;
}
// kevent thread has a retain, so no contention with _disableNotification since we can't dealloc
@@ -527,19 +604,21 @@
a race condition between kqueue and wait. Since we know the child has exited, we can allow waitpid
to block without fear that it will block indefinitely.
*/
- int wait_flags = 0;
int ret, status;
- // keep trying in case of EINTR
- ret = waitpid(_processIdentifier, &status, wait_flags);
- while (-1 == ret && EINTR == errno)
- ret = waitpid(_processIdentifier, &status, wait_flags);
+ ret = HANDLE_EINTR(waitpid(_processIdentifier, &status, 0));
+ // happens if you call waitpid() on the child process elsewhere; don't do that
+ if (-1 == ret)
+ perror(__func__);
+
if (0 == ret)
NSLog(@"*** ERROR *** task %@ (child pid = %d) still running", self, _processIdentifier);
_processIdentifier = -1;
+ _internal->_terminationReason = WIFSIGNALED(status) ? NSTaskTerminationReasonUncaughtSignal : NSTaskTerminationReasonExit;
+
ret = WIFEXITED(status) ? WEXITSTATUS(status) : 1;
bool swap;
This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.
|