19 November 2013 by Published in: Code, iphone, rants 9 comments

So back in the dark ages, we registered to receive notifications like this:

-[NSNotificationCenter  addObserver:selector:name:object:]

In other words, the target-action pattern. When the notification is received, call this selector (action) on this target. And all was well.

Then in iOS 4, blocks (closures) were added to iOS. And it was the hip cool thing to add block versions of everything. Blocks in a box! Blocks in their socks! After all, the NSNotification (observer) pattern is sort of deferred execution, right?

So a blocks-based API was added to NSNotificationCenter. Here it is:

-[NSNotificationCenter addObserverForName:object:queue:usingBlock:]

This turned out to be a terrible idea. This could be, I think, the single biggest API blunder in iOS. (Except maybe iCloud Core Data.) I have debugged issues that were root-caused to this misleading API more than ten times. I have lost more than four weeks to this API. I have no less than six radars open about it.

All of this is in spite of the fact that it is banned from any codebase I work on.

For those who accuse me of drinking the Apple kool-aid, rest assured, there is nothing pro-Apple in this blog post. Read on for 2500 words of pure complaining.

So how bad could it be?

We’re going to be good little TDD citizens for the duration of this blog post, so let me show you the test first:

- (void)testExample
{
    for(int i =0; i < 5; i++) {
        YourAttempt *attempt1 = [[YourAttempt alloc] init];
        [[NSNotificationCenter defaultCenter] postNotificationName:notificationName object:nil];
        XCTAssertEqual(counter, i+1, @"Unexpected value for counter.");
        XCTAssertEqual(1, attempt1.localCounter, @"Unexpected value for localCounter.");
    }
}

So this test is trivial:

  • We create an Attempt object
  • We post a notification
  • We check that the notification increases our global counter variable.
  • We check that the notification increases the object’s localCounter variable.

Now at this point you’re probably saying: “This looks sort of simple.” If that is you, just skip the rest of this blog post, pull down the GitHub repository, type your solution into YourAttempt.m, and press Command-U. But not until you’re really, really sure it’s right. Don’t worry, I’ll wait. It’ll be more fun if you fail first.

Still reading? You lazy bum. Making me do all the work.

If at first you don’t succeed

Our first attempt really is trivial:

@interface Attempt1() {
}
@end
@implementation Attempt1
-(id)init {
    if (self = [super init]) {
        [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) {
            int oldCounterValue = counter;
            counter++;
            self.localCounter++;
            NSAssert(counter==oldCounterValue+1, @"Atomicity guarantee violated.");
        }];
    }
    return self;
}
@end

That should do the trick! Except:

"3" is not equal to "2" - Unexpected value for counter.
"6" is not equal to "3" - Unexpected value for counter.
"10" is not equal to "4" - Unexpected value for counter.
"15" is not equal to "5" - Unexpected value for counter.

Can you figure out what’s going on here? We’re looking for 1,2,3,4,5 in our counter —but we’re getting 1,3,6,10,15. So why those numbers?

Well, that sequence is called the triangular numbers. And that pattern is a dead giveaway. See, the first time we post the notification, it runs once. So our counter is 1. But the second time, it runs twice. So our counter is 3. And the third time, it runs thrice. Now our counter is six.

Now you might say: “So what if my notification runs a few times too many? I’m not stupid enough to use global variables.” Well, maybe. If you’re using things like the camera, the microphone, anything in your app delegate, etc., then you are, actually, using global variables. But forget about that for a minute–imagine what would happen if we picked a function at random from your codebase and ran it twice instead of once. What would happen? We could try to insert two objects into your database, or delete an object twice. We could pop an extra view controller. We could repeat your online purchase. Who knows?

In fact, the really dangerous thing about this bug is that practically anything can happen. This bug can present as any bug. Any bug report you get can be this bug. That’s bad. That’s really bad. And that’s why I have spent so much time on bugs that turned out to be this one. Just to give some real-life bug reports:

  • “Whenever I try to take a picture, the lens doesn’t open.”
  • “If I go to Screen A, leave it, and come back, the button on Screen B does something really strange.”
  • “After I pick a photo from my photo library, the app works fine. For about 20 seconds. Then it crashes. But I can only reproduce this once per testing session. I have to wait until tomorrow to catch it again.”

Do these sound like repeated notification bugs? No. That’s why it’s so scary.

So: let’s not get notifications twice. Well, obviously! We forgot to unregister for the notification. Let’s do that.

A very selfish attempt

@interface Attempt2() {
    id cleanupObj;
}
@end
@implementation Attempt2
-(id)init {
    if (self = [super init]) {
        cleanupObj = [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) {
            int oldCounterValue = counter;
            counter++;
            self.localCounter++;
            NSAssert(counter==oldCounterValue+1, @"Atomicity guarantee violated.");
        }];
    }
    return self;
}
- (void)dealloc {
    [[NSNotificationCenter defaultCenter] removeObserver:cleanupObj];
}
@end

Run that and… bam! We’re done!

"3" is not equal to "2" - Unexpected value for counter.
"6" is not equal to "3" - Unexpected value for counter.
"10" is not equal to "4" - Unexpected value for counter.
"15" is not equal to "5" - Unexpected value for counter.

…what? The exact same result? What’s going on here?

Well, the same thing as last time. Even though we have written code in our dealloc to remove our Attempt from the notification, the notification keeps getting called. So why? Did we get the syntax wrong or something?

No, the syntax is fine. What’s wrong is that dealloc never gets called. Why not?

Well, when you declare a block, the compiler goes in and scans what the block does. This is because if you write some code like id x = @(42); and then declare a block that uses x, that block might long outlive the variable x. So x needs to stick around at least as long as the block does.

So the culprit here is that our block contains the expression

self.localCounter++;

which is syntactically equivalent to

[self setLocalCounter:[self localCounter]+1];

Which contains not one, but two references to self. So the block, as soon as it is declared, acquires an owning reference to self, because the block needs self to run. And since NSNotificationCenter owns the block, and the block owns self, self will never be deallocated. Ever.

Hey, you want to know what else is scary? This code builds cleanly. Not a peep from the compiler; not a peep from Clang Static Analyzer. In fact, every buggy code listing you see in this post gets a clean bill of health from both. This is in spite of the fact that LLVM has a warning for this very bug. You might have seen it:

Capturing self strongly in this block is likely to lead to a retain cycle

Clang is just not powerful enough, in its present form, to find this type of bug. Consider yourself alarmed.

Well, the solution is trivial: we just remove self from our block.

Practicing selflessness

@interface Attempt3() {
    id cleanupObj;
}
@end
@implementation Attempt3
-(id)init {
    if (self = [super init]) {
        cleanupObj = [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) {
            int oldCounterValue = counter;
            counter++;
            _localCounter++;
            NSAssert(counter==oldCounterValue+1, @"Atomicity guarantee violated.");
        }];
    }
    return self;
}
- (void)dealloc {
    [[NSNotificationCenter defaultCenter] removeObserver:cleanupObj];
}
@end

And… tada!

"3" is not equal to "2" - Unexpected value for counter.
"6" is not equal to "3" - Unexpected value for counter.
"10" is not equal to "4" - Unexpected value for counter.
"15" is not equal to "5" - Unexpected value for counter.

Wait, still?

Yeah, still. So this is actually the same problem as before, it’s just implicit. You see, if we’re going to keep the _localCounter ivar around, we also have to keep self around to put it in.

Thus sayeth the documentation:

When a block is copied, it creates strong references to object variables used within the block. If you use a block within the implementation of a method [and] you access an instance variable by reference, a strong reference is made to self

The documentation goes on to suggest:

To override this behavior for a particular object variable, you can mark it with the __block storage type modifier.

Ah, so that should be straightforward! We’ll just mark the _localCounter with __block.

It’s a __block party

@interface Attempt4() {
    id cleanupObj;
    __block int _localCounter;
}
@end
@implementation Attempt4
-(id)init {
    if (self = [super init]) {
        cleanupObj = [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) {
            int oldCounterValue = counter;
            counter++;
            _localCounter++;
            NSAssert(counter==oldCounterValue+1, @"Atomicity guarantee violated.");
        }];
    }
    return self;
}
- (void)dealloc {
    [[NSNotificationCenter defaultCenter] removeObserver:cleanupObj];
}
@end

How bad could it be?

"3" is not equal to "2" - Unexpected value for counter.
"6" is not equal to "3" - Unexpected value for counter.
"10" is not equal to "4" - Unexpected value for counter.
"15" is not equal to "5" - Unexpected value for counter.

Hmm… ok, now you’re just messing with me, Apple. You’ve told me doing this would solve my problem. What gives?

What gives is that this documentation flits back and forth between whether or not it’s talking about an object variable, as opposed to, I guess, the other kind. See:

it creates strong references to object variables used within the block… If you access an instance variable by reference, a strong reference is made to self;… To override this behavior for a particular object variable, you can mark it with the __block storage type modifier.

In other words, at the beginning and (crucially) the workaround, they’re talking about object variables. Meanwhile what we’re using is just an integer.

OK so maybe we can convert our code to use an object variable, and then the workaround will work?

When the documentation fails

@interface Attempt5() {
    id cleanupObj;
    __block NSNumber *counterObj;
}
@end
@implementation Attempt5
-(id)init {
    if (self = [super init]) {
        cleanupObj = [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) {
            int oldCounterValue = counter;
            counter++;
            counterObj = @(counterObj.intValue + 1);
            NSAssert(counter==oldCounterValue+1, @"Atomicity guarantee violated.");
        }];
    }
    return self;
}
- (void)setLocalCounter:(int)localCounter {
    counterObj = @(localCounter);
}
- (int)localCounter {
    return counterObj.intValue;
}
- (void)dealloc {
    [[NSNotificationCenter defaultCenter] removeObserver:cleanupObj];
}
@end

And with a Cmd-U, we’ll get…

"3" is not equal to "2" - Unexpected value for counter.
"6" is not equal to "3" - Unexpected value for counter.
"10" is not equal to "4" - Unexpected value for counter.
"15" is not equal to "5" - Unexpected value for counter.

Seriously? How did this get through QA? Did anyone even test this? What sort of person even wrote this documentation?

Well, the sort of person who doesn’t read compiler specifications. Noobs. Because, in-between WWDC videos, documentation, sample code, and you know, actually writing code, I’ll bet you have nothing better to do than browse random technical specs on clang.org.

Because a 27-page document that doesn’t even rate a mention in the Clang documentation table of contents very clearly states buried in the middle of Section 7.5:

The inference rules apply equally to __block variables, which is a shift in semantics from non-ARC, where __block variables did not implicitly retain during capture.

Go ahead and try to make sense of that sentence, I dare you.

No? So essentially this is compilerese for “We changed it.”

Back in the Land Before ARC, the use of __block would prevent a block from retaining a variable. However in the ARC world, we have a whole constellation of memory keywords: __strong, __weak, __autoreleasing, __unsafe_unretained… And so when they introduced these, they decided to divorce __block from the memory keywords, so you could have, say, __unsafe_unretained __block id foo; if you like. And just like every other kind of variable, the default, implicit memory keyword for __block is __strong.

So that’s why it doesn’t work. Now, you might say, let’s just declare __counterObj as memory type __weak. But of course, then it would have no strong reference, and vanish into the æther. Nor can we create a weak reference to our counter object immediately prior to the block and use that, because we must eventually set our counter to a new value.

So instead we will improvise:

Your invariants may vary

@interface Attempt6() {
    id cleanupObj;
}
@end
@implementation Attempt6
-(id)init {
    if (self = [super init]) {
        __weak Attempt6 *mySelf = self;
        cleanupObj = [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) {
            int oldCounterValue = counter;
            counter++;
            NSAssert(counter==oldCounterValue+1, @"Atomicity guarantee violated.");
            mySelf.localCounter++;
        }];
    }
    return self;
}
- (void)dealloc {
    [[NSNotificationCenter defaultCenter] removeObserver:cleanupObj];
}
@end

There are a lot of things not to like about this solution: it relies on using a public interface to access our own members, for example. So any notification you process will need to use an API with public scope visibility, which means you will have to expose one. And hope that nobody uses it.

But anyway, it should work, right? (Or have you figured out this gimmick by now?)

"3" is not equal to "2" - Unexpected value for counter.
"6" is not equal to "3" - Unexpected value for counter.
"10" is not equal to "4" - Unexpected value for counter.
"15" is not equal to "5" - Unexpected value for counter.

SERIOUSLY. MUST. KILL. COMPILER.

OK, what is wrong here? I’ll give you a hint: if you test in Release mode, it works fine. It only fails in Debug mode.

Give up?

Here’s the answer:

NSAssert(counter==oldCounterValue+1, @"Atomicity guarantee violated.");

See, NSAssert is a macro. A macro that expands to this:

do {
    __PRAGMA_PUSH_NO_EXTRA_ARG_WARNINGS
    if (!(condition)) {
        [[NSAssertionHandler currentHandler] handleFailureInMethod:_cmd
        object:self file:[NSString stringWithUTF8String:__FILE__]
            lineNumber:__LINE__ description:(desc), ##__VA_ARGS__];
    }               \
        __PRAGMA_POP_NO_EXTRA_ARG_WARNINGS
} while(0)

See there? Big fat self. Ergo, retain cycle, ergo test failure.

The Final Solution

Here’s the final answer, using the lesser-known NSCAssert function. Which, by the way, is not supposed to be used in Objective-C:

This macro should be used only within C functions.

But hey, we’ve already demonstrated that this documentation is buggier than MobileMe on launch day.

@interface Attempt7() {
    id cleanupObj;
}
@end
@implementation Attempt7
-(id)init {
    if (self = [super init]) {
        __weak Attempt7 *mySelf = self;
        cleanupObj = [[NSNotificationCenter defaultCenter] addObserverForName:notificationName object:nil queue:nil usingBlock:^(NSNotification *note) {
            int oldCounterValue = counter;
            counter++;
            NSCAssert(counter==oldCounterValue+1, @"Atomicity guarantee violated.");
            mySelf.localCounter++;
        }];
    }
    return self;
}
- (void)dealloc {
    [[NSNotificationCenter defaultCenter] removeObserver:cleanupObj];
}
@end

So now looking at a working implementation, is this the sort of thing you would come up with left to your own devices? I’m guessing no.

And you’re in good company

The documentation lists no fewer than six sample code projects underneath the Notifications-with-blocks API. And five out of six Apple model projects are wrong. Let that sink in.

  • AVCam – On line 89 of AVCamCaptureManager.m, Apple uses the syntax __block id weakSelf = self;, apparently under the impression that this breaks the reference cycle. No instance of this class can ever be deallocated. A GitHub search reveals some 77 repositories contain some version of this class.
  • AVLoupe – In this project, Apple straight-up never removes the observer from the NSNotificationCenter. How’s that for sample code?
  • AVMovieExporter’s VideoLibrary crashes on dealloc for unrelated reasons. Bonus points are awarded for a novel use of __unsafe_unretained __block to work around the retain cycle that will cause an EXC_BAD_ACCESS crash if the retain cycle is broken and a notification is subsequently posted.
  • AudioTapProcessor – boldly using self inside the block. Bonus points awarded for attempting to remove the observer in viewDidUnload in spite of the fact that viewDidUnload is never called in iOS 7, which was in beta 5 when this code was published. In fairness, it might work on iOS 6. I didn’t test it.
  • TaggedLocations (a.k.a. SimpleCoreDataRelationships) is the only project I can’t find anything wrong with. So good job, anonymous developer of this sample project, you’re in the top 13% of Cocoa engineers at Apple.

Killing the beast

For the rest of us, just stay far away from this API. cd into your code’s directory and type this to find out how bad your codebase is:

grep -R "addObserverForName:.*object:.*queue:.*usingBlock.*" --include="*.m" .

This API is banned from my codebases. There is certain third-party code where I will tolerate grandfathering it in, but not without a thorough audit. And the audit, unfortunately, usually finds something.

Alternatives

Nick Lockwood has been working on an alternative API over here.

Jonathan Willing has an alternative API over here.

Like this post? Contribute to the coffee fund so I can write more like it.

Comments

  1. Marc
    Tue 19th Nov 2013 at 10:59 pm

    “AVCam – On line 89 of AVCamCaptureManager.m, Apple uses the syntax __block id weakSelf = self;, apparently under the impression that this breaks the reference cycle. No instance of this class can ever be deallocated. A GitHub search reveals some 77 repositories contain some version of this class”

    If you do weakSelf = nil; inside the block after you are done it will break the retain cycle. You have to be careful if you use dispatch_asyncs that reference the weakSelf, make sure to nil it out inside the dispatch and not right after it.

    Apple sample code is generally of low quality and should not be trusted.

    Thank you for the warning about NSAssert, most of us did not see that one coming…

  2. Wed 20th Nov 2013 at 12:12 am

    The use of __weak is documented in Apple’s “Transitioning to ARC Release Notes” and has been since ARC was first released. There are lots of easy to understand examples. That was fine when ARC came out, but they really should have copied or linked to that from somewhere else.

    Since I first started iOS programming, I’ve been using ZAssert
    http://www.cimgf.com/2010/05/02/my-current-prefix-pch-file/ by Marcus Zarra. It doesn’t reference self.

    I would consider the real bug to be on NSAssert, which should not reference self. Or be documented to not be used in a block.

  3. Shripada
    Wed 20th Nov 2013 at 12:17 am

    Nice article!. Retain cycle with blocks is a subtle trap to figure out and fix and this NSNotificationCenter API takes it to new heights!. I wonder why LLVM automatically create weak references to ‘self’ within a block by default. Not sure if it is too difficult for LLVM folks to bring it in.

    Thanks
    Shripada

  4. Aaron Sarazan
    Wed 20th Nov 2013 at 12:59 am

    Fyi, you don’t have to maintain a public interface to access that variable in mySelf. You can just deref it with the arrow operator as such: mySelf->_localCounter++;

    The rest of this is terrifying.

  5. Drew Crawford
    Wed 20th Nov 2013 at 1:00 am

    You can only deref with the arrow operator if the ivar is declared @public.

  6. Wed 20th Nov 2013 at 1:17 am

    Aaron: Actually you can’t, because you can’t dereference a weak pointer (it might become nil at any point, so it’s always unsafe to dereference it). However, inside the block you can create a strong reference, and dereference THAT one only if it is not nil.

    __weak __typeof(self) weakSelf = self;
    [something doSomething:^{
    __strong __typeof(weakSelf) strongSelf = weakSelf;
    if(strongSelf)
    strongSelf->_ivar++;
    }];

    Also, this whole blog article is rather over the top. Yes, memory management in blocks is difficult. That’s the price to pay for not having a garbage collector. That doesn’t mean you can just ignore the rules, and it doesn’t mean that block-using APIs are bad. It just means you don’t know how to use blocks. (and I know you don’t know how to use block because you made an instance variable __block. That doesn’t even mean anything. And you didn’t do what the documentation said — it implied that you should’ve made the reference to self weak, not the reference to the ivar.)

    It’s also worth to note that if you don’t unregister from notifications with the target-action API, you’ll crash. There’s plenty of code using that API incorrectly too, and that’s just as bad.

  7. Aaron Daub
    Wed 20th Nov 2013 at 1:43 am

    It looks like you can -> down to private/protected ivars but Xcode doesn’t like it when you dereference a weak pointer with ->.

    You have to create a strong variable within the block that’s a copy of the weak reference, and dereference that.

  8. Wed 20th Nov 2013 at 1:46 am

    I wonder why LLVM automatically create weak references to ‘self’ within a block by default. Not sure if it is too difficult for LLVM folks to bring it in.

    Because that would be at least as wrong. All Objective-C object references are captured strongly, that’s the API contract. If self was special cased to be weak, this would break:

    [[thing sendRequest:r withCallback:^(id value) {
    [self parseValue:value];
      [self save];
    }];

    Since self is captured strongly, we’re guaranteed to live at least as long as this request, even if all other references to the self object have been released except this block, so that we can definitely handle the response unless the request is explicitly cancelled.

    Special casing self to be weak wouldn’t help in this situation, either:

    id manager = …;
    [manager onFailure:^{
    [manager cleanup];
    }];

    Retain cycle!

    So maybe we can go so far as to say all objc values should be captured weakly?

    NSArray *array = @[@1];
    [thing sendRequest:r withCallback:^(id response){
    [response doSomethingWithNonNilObject:array];
    }];

    Nope, crash.

    So, the current behavior of strong capture of every Objective-C object is the sanest solution, but is problematic since we don’t have garbage collection to take care of cycles. Harsh life as a programmer. We have to learn our tools very thoroughly and learn how to handle these cases correctly instead. ARC made life easier but it’s not a panacea.

  9. Mon 09th Dec 2013 at 6:45 pm

    “On line 89 of AVCamCaptureManager.m, Apple uses the syntax __block id weakSelf = self;, apparently under the impression that this breaks the reference cycle. No instance of this class can ever be deallocated. A GitHub search reveals some 77 repositories contain some version of this class.”

    Yeah, I’ve noticed!

    Had to go into my code base and dig this one up. I added this kind of hacky piece of code and call it manually to make the damned AVCamCaptureManager dealloc:

    (void) removeLeakyObservers
    {
    NSNotificationCenter *notificationCenter = [NSNotificationCenter defaultCenter];
    [notificationCenter removeObserver:[self deviceConnectedObserver]];
    [notificationCenter removeObserver:[self deviceDisconnectedObserver]];
    }

    And thanks for this dive into the (at least for me) good old weakSelf & strongSelf dance that came as a little nasty side effect of ARC’s new way of handling variables defined as __block. Though the ARC documentation does describe (with examples) this well, it is a nasty little change in the way things worked earlier.

Add comment

Copyright © 2011 Drew Crawford, All Rights Reserved
Powered by WordPress

Page optimized by WP Minify WordPress Plugin