
NSNotificationCenter with blocks considered harmful - dcaunt
http://sealedabstract.com/code/nsnotificationcenter-with-blocks-considered-harmful/
======
AshFurrow
There's nothing really magical about this – don't cause retain cycles,
everyone knows __block semantics changed with ARC, keep the returned value
from the block-based NSNotificationCenter method, etc. Standard stuff.

The only thing that surprised me was the reference cycle in NSAssert. Then
again, given Apple's poor regard for TDD, that shouldn't surprise me.

~~~
drewcrawford
> everyone knows __block semantics changed

Everyone except the authors of the "Blocks Programming Topics" and the authors
of the AVCamCaptureManager sample code. Maybe.

FYI the reason I included the digression about the __block change specifically
is that I was recently fixing bugs on a project that uses AVCamCaptureManager,
and Apple's mis-use of __block in that class, this is not a joke, through a
series of corner cases, caused the status bar to unexpectedly and
nondeterminisitically change color about 5% of the time on an unrelated
screen.

Saying "everybody should already know how to do weak references" is great in
theory. But if you are fielding weird reports for unreproducible status bar
issues and it occurs to you at _any time in the first hour_ that maybe Jim
from the next cubicle used buggy sample code for a video recording feature on
another screen you are a WAY better software developer than I am.

Of course this is standard stuff. But unlike a lot of standard stuff, this one
can go undetected for long periods, and crop up in _very_ unexpected places.
That's the problem.

~~~
matwood
Apple's sample code is terrible. I've always assumed it is written by interns
because it is generally bug ridden and often _not_ the right way to do
something.

~~~
chc
Do you mean the code examples in articles or the projects that you can
download? Because I haven't looked at their downloadable samples in quite some
time, but their docs always seem alright to me.

~~~
seandougall
Both, in my experience. The downloadable code is usually a rush job and has
all sorts of sloppiness and missed edge cases. The code in the docs is usually
cleaner (because it's just excerpts), but doesn't always get updated when APIs
change.

------
chrisdevereux
This really applies equally to any block that captures self. Blocks are, IMHO,
the one place where ARC really falls down versus garbage-collection.

~~~
gilgoomesh
Yes, it's just a retain cycle. Self retains block, block retains self.

The only thing that makes this case confusing is that self only holds the
block implicitly (via the notification center instead of any obvious ivar).
Otherwise it's a design pattern that every reference counted language user
must understand to avoid memory leaks. Weak references to self in blocks are a
_very_ common design pattern that all Objective-C programmers should know.

~~~
antimagic
Not to mention that using 'self' in a block is just not a good idea to start
with. Conceptually a block is not a method, so it doesn't have a well-defined
self. Instead we are relying on the lexical scope capturing of blocks. The
thing is, the documentation is very clear about what happens when you access
self in a block[1]: "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:

    
    
        If you access an instance variable by reference, a strong reference is made to self;
    
        If you access an instance variable by value, a strong reference is made to the variable."
    

Right. So if you use self or an instance variable, you've just created a
strong reference. In Drew's example, as he is relying on the object going out
of scope, that will still leave the reference held by the block, which is
itself held by the NotificationCenter. As with all reference cycles, the way
out is to get the NotificationCenter to stop observing that notification, or
as the Apple documentation says, use a local variable that takes the value of
self, and use that local variable inside the block. This is the solution that
Drew presents as Attempt6, but quite frankly would have been my very first
attempt when the test failed. The stuff about __block is a red-herring, and
from the documentation goes in the opposite direction that Drew wants, forcing
a strong reference rather than removing it.

Drew also gets bitten by NSAssert using self. I would suggest that it is
NSAssert that is doing something dodgy here - it's trying to have an implicit
self, as though it was a method, when it most definately isn't a method - and
Drew just got bitten by the impedence mismatch. Macros - just say no.

------
pjungwir
The book _Programming iOS 6_ by Matt Neuburg recommends the "weak-strong
dance" for this, like so (p. 326):

    
    
        __weak MyClass* wself = self;
        self->observer = [[NSNotificationCenter defaultCenter]
                           addObserverForName:@"heyho"
                           object:nil queue:nil
                           usingBlock:^(NSNotification *n) {
                             MyClass *sself = wself;
                             if (sself) {
                               NSLog(@"%@", sself);
                             }
                           }];
    

Do other people agree this fixes the problem?

Also, it's my understanding closures only cause retain cycles if the method
taking the block _copies_ the block, per the docs: "When a block is _copied_ ,
it creates strong references to object variables used within the block."
Therefore this dance is not necessary for e.g. [NSURLConnection
sendAsynchronousRequest:queue:completionHandler:]. Is that correct?

------
kybernetyk
A little off topic:

If you over-do it then NSNotifications easily can become distributed goto.
Once I had to add features to a Mac application that made heavy (ab)use of
NSNotifications. Following code paths was a nightmare.

I tend to prefer the delegate pattern because there the relationships between
objects are clear. Even if it means more work (creating glue code) - your
sanity is worth it.

------
obiterdictum
On a related note, I've run into a similar problem in C++, but opposite
effect. A lambda _won 't_ keep my object alive if I try to capture a
shared_ptr _member_ , because C++ lambdas, similarly to blocks, by default
capture implicit reference to "this", rather than individual instance
variables.

[http://stackoverflow.com/q/7764564/23643](http://stackoverflow.com/q/7764564/23643)

------
pothibo
Nice post! Are you always testing Apple's implementation details in your TDD?
To me it seems the actual notification submission shouldn't be tested, only
that it was called with the proper value?

Just a heads up though: The code snippet were hard to read because of the
indentation. May I suggest

    
    
      cleanupObj = [[NSNotificationCenter defaultCenter]
                  addObserverForName:notificationName
                  object:nil
                  queue:nil
                  usingBlock:^(NSNotification *note) {
                    // Code!
                  }];
    

It might not be your coding style but in very narrow container, it makes it
easier to read ;)

------
Strilanc
There are three related mistakes contributing to this bug:

1\. The test is depending on dealloc being done eagerly.

2\. The Attempt class puts unsubscribe code in its dealloc method.

3\. The Attempt class does not force callers to control its lifetime.

I'll go into more depth on these.

1\. Cleanup is often deferred. Dealloc may only run AFTER the test instead of
DURING the loop (e.g. I think NSArray defers releasing its items in some
cases). Tests that depend on cleanup occurring eagerly, without explicitly
waiting or forcing it, are brittle. Given that Attempt uses dealloc to
unsubscribe, and we are testing that it unsubscribed, we should at the very
least allocate it in an autoreleasepool that ends before we test that it
unsubscribed.

2\. Putting unsubscribe code in dealloc is a great way to upgrade minor memory
leak bugs into major behavior bugs. The article gives fantastic examples of
this happening many different ways. Don't rely on the object happening to go
out of scope at the right time. If your object's lifetime is controlling
whether or not side effects occur, that's a bad situation to be in.

3\. Someone has to be responsible for when unsubscribing happens (since we
took it away from dealloc), and the someone most in a position to know when is
the caller. We should force them to tell us our subscription lifetime. I like
to control lifetimes with cancellation tokens [1], so I'd write the attempt
class like this:

    
    
        @implementation Attempt
        -(instancetype) initUntil:(TOCCancelToken*)untilCancelledToken {
            if (self = [super init]) {
                id cleanupObj = ... addObserverForName stuff from Attempt1 ...
                [untilCancelledToken whenCancelledDo:^{ 
                    [NSNotificationCenter.defaultCenter removeObserver:cleanupObj];
                }];
            }
            return self;
        }
        - (void)setLocalCounter:(int)localCounter { ... }
        - (int)localCounter { ... }
        @end
    

Here's what the test should look like:

    
    
        -(void)testExample {
            for(int i =0; i < 5; i++) {
                TOCCancelTokenSource* c = [TOCCancelTokenSource new];
                Attempt *a = [[Attempt alloc] initUntil:c.token];
                [NSNotificationCenter.defaultCenter postNotificationName:notificationName object:nil];
                [c cancel];
                
                XCTAssertEqual(counter, i+1, @"Unexpected value for counter.");
                XCTAssertEqual(1, attempt1.localCounter, @"Unexpected value for localCounter.");
            }
        }
    

Hopefully I got that right. I don't have a mac nearby to test it out at the
moment, and I've never used NSNotificationCenter before.

1: [http://twistedoakstudios.com/blog/Post7391_cancellation-
toke...](http://twistedoakstudios.com/blog/Post7391_cancellation-tokens-and-
collapsing-futures-for-objective-c)

~~~
e28eta
I disagree with your mistake #1, and therefore also with the second one. In
ARC, dealloc is done eagerly. So, I don't have a problem with unsubscribing in
dealloc.

I dislike that receiving the notification mutates global state though. I can't
clearly articulate it, but that's why I think the caller should have explicit
control over when the behavior is active. I think notification subscriptions
that last for the lifetime of the object should probably be related to the
object's internal state.

Finally, I went to the clang docs regarding timing of dealloc, and as I read
it, they shoot for immediate deallocation, but let it slide for local
variables:

    
    
        Precise lifetime semantics
        
        In general, ARC maintains an invariant that a retainable object pointer
        held in a __strong object will be retained for the full formal lifetime
        of the object. Objects subject to this invariant have precise
        lifetime semantics.
        
        By default, local variables of automatic storage duration do
        not have precise lifetime semantics. Such objects are simply strong
        references which hold values of retainable object pointer type,
        and these values are still fully subject to the optimizations
        on values under local control.
        
        Rationale
        Applying these precise-lifetime semantics strictly would be prohibitive.
        Many useful optimizations that might theoretically decrease the
        lifetime of an object would be rendered impossible. Essentially,
        it promises too much.
    
        A local variable of retainable object owner type and automatic storage
        duration may be annotated with the objc_precise_lifetime attribute to
        indicate that it should be considered to be an object with precise lifetime
        semantics.
        
        Rationale
        Nonetheless, it is sometimes useful to be able to force an object to
        be released at a precise time, even if that object does not appear
        to be used. This is likely to be uncommon enough that the syntactic
        weight of explicitly requesting these semantics will not be burdensome,
        and may even make the code clearer.

------
archagon
This article reminds me to brush up on my blocks! Good to have all these error
cases all in one place. :)

------
dmishe
Nice, just the other day I used NSAssert in C function and got weird crash on
self.

------
adamcavalli
typeof(self) __weak weakSelf = self;

self.block = ^{ typeof(self) strongSelf = weakSelf; if (strongSelf) { // ... }
};

------
jheriko
I consider the whole of Objective-C harmful.

It lives only in the 'platform' layer of my code. Grudgingly because Apple
enforce it.

It could be worse... they could have pulled a Google and used Java - then held
back the tools they develop e.g. the Java VM because they are 'dangerous' and
suggest that using native code is 'bad' if it is just for performance or
cross-platform reasons.

At least the interoperability with sane programming languages in Objective-C
is excellent.

I'd point out the vast majority of memory management issues I have is when
using someone's refcounting or gc scheme. new and delete are exactly what i
want all of the time. i like to tell the machine what to do and i have
developed non-trivial software without leaks /before testing for leaks/
because once you have some practice with new and delete it becomes easy and
you will never look back...

As a mechanism for broadcasting information throughout an app
NSNotificationCenter is slower, more complicated and (apparently) more
dangerous than my hand rolled code. That should be shocking and its counter to
the common idea that 3rd party and especially OS libraries should be better...
there are a number of cases where they measurably aren't.

(The overhead of the objective C message mechanism - or even the RTTI
mechanism which is a small part of that, exceeds that of adding or reading
something from a well implemented threadsafe data structure (which is the
first step to either of these things in most cases))

~~~
jheriko
Any explanation for the downvotes? I concede the value of rapid application
development, but objective-c itself doesn't provide this at all - rather the
libraries like UIKit or Cocoa coupled with the Xcode development environment
do this.

Objective-C is measurably bad for my code. I have reams of data to support
this... and its all trivially reproducable.

~~~
chrisdevereux
I didn't downvote you, but you directed a lot of loosely-connected negative
assertions at a single target without really backing any of them up.

Whatever you intended, it came across as a bit of a rant.

~~~
jheriko
when i mention the slowness of the message mechanism this is both by design of
Objective-C (for runtime features) and very measurable. this is why, for
example, NSArray becomes prohibitively slow when you want to iterate across
1000s of members.

Objective-C is nice, but its slow by design, and this rules it out in a
concrete way for a lot of the work I've done.

Its sad when optimisation comes down to 'knowing better than the compiler'
only because of language constraints. (even C has this problem with struct
layout for example - its just prevalent when using Objective-C/Cocoa (Touch))

