views:

40

answers:

1

Hello! I have a simple class that looks a bit like this:

@protocol Recorder
@property(BOOL) isRunning;
- (void) start;
- (void) stop;
@end

And the method implementations:

- (void) start {
    if (running)
        return;
    …
    running = YES;
}

- (void) stop {
    if (!running)
        return;
    …
    running = NO;
}

And I started thinking about thread safety. The current solution is not thread safe, right? How about this:

- (void) start {
    @synchronized(self) {
        if (running)
            return;
        …
        running = YES;
    }
}

Is this correct, provided that the -stop method is also synchronized? I don’t like the extra nesting introduced by @synchronized, though. Would explicit lock work?

- (void) stop {
    [startStopLock lock];
    if (running)
        return;
    …
    running = YES;
    [startStopLock unlock];
}

Or could I do even this?

enum { Running, Stopped };
NSConditionLock *startStopLock;

- (void) start {
    if (![startStopLock tryLockWithCondition:Stopped])
         return;
    …
    [startStopLock unlockWithCondition:Running];
}

Is this solution correct? Would you do things differently?

+1  A: 

What language is this? You are correct, the first version is not thread safe.

The synchronized version is thread safe. With an explicit lock you need to be careful not to miss the unlock on the early return path.

If you have access to locked xchg intrinsics, you can do it fairly easily with an atomic exchange operation. cmpxchg also works.

start() {
    if (locked_xchg(running, YES) == YES) {
        // the old value was YES, so nothing to do
        return
    }
Dave Dunn
This is Objective-C on iOS. Thank you for the `locked_xchg` hint, I have `OSAtomicTestAndSet` which should be about the same.
zoul
No problem. This is a classic mistake that people make. Unfortunately our Jinx product doesn't run on the iphone. Otherwise I'd recommend using it to help catch these races.
Dave Dunn