7 ms·

NSNotificationCenter with blocks considered harmful

90 points by dcaunt 13y ago

chrisdevereux 13y ago
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 13y ago
  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 13y ago
    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.
  - chrisdevereux 13y ago
    Using blocks is the one bit of Objective-C that still reminds me of the old MRR style. Just like with retain/release/autorelease, it's one thing understanding the pattern, it's another ensuring that your entire program conforms to it. Anywhere that you capture nontrivial objects is a possible failure point. ReactiveCocoa has some nice patterns for avoiding block callbacks altogether. Eg: [myTarget rac_liftSelector:@selector(receiveCallback:) withSignals:mySignal, nil]; will capture a weak reference to the target, invoke the callback whenever 1 or more events fire and automatically unsubscribe when the target deallocates.
AshFurrow 13y ago
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.
- AshFurrow 13y ago
  By the way, you can prevent the retain cycle in NSAssert using libextobjc's @weakify and @strongify macros.
  - dcaunt 13y ago
    Indeed, the NSAssert cycle surprised me too. libextobj is great.
- deleted 13y ago
  [deleted]
- seivan 13y ago
  Pretty much my take on it as well. Nothing out of the ordinary here except the NSAssert, but I got that before the summer. A hack to get around the self inside the macro https://github.com/seivan/SHAlertViewBlocks/blob/develop/Example/Example/Example-Prefix.pch#L9-L23 https://github.com/seivan/SHAlertViewBlocks/blob/develop/Exa...
- stigi 13y ago
  I'd recommend looking at @weakify and @strongify that come with libextobjc (https://github.com/jspahrsummers/libextobjc/ https://github.com/jspahrsummers/libextobjc/). Handy little helpers to avoid retain cycles with all kind of blocks.
- mackey 13y ago
  I agree and disagree. For me, I agree this is standard stuff. If you are writing this stuff everyday, this block song and dance is basically taken care of with muscle memory. Part of my job involves working with people from other companies who aren't as strong with objective-c. There are a lot of people that have trouble blocks for whatever reasons. I recently had a guy from another company act almost surprised that we were using "advanced features like blocks". We were a little shocked. But it just shows that not everyone is comfortable with them. BUT, I think a better approach from the author wouldn't be to tell people not to use blocks w/ NSNotificationCenter but to make sure they know the in and outs of blocks, because blocks are amazingly powerful tools and they aren't going anywhere.
kybernetyk 13y ago
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.
pothibo 13y ago
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 ;)
obiterdictum 13y ago
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
jheriko 13y ago
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 13y ago
  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 13y ago
    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.
    - 13y ago
archagon 13y ago
This article reminds me to brush up on my blocks! Good to have all these error cases all in one place. :)
Strilanc 13y ago
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-tokens-and-collapsing-futures-for-objective-c http://twistedoakstudios.com/blog/Post7391_cancellation-toke...
- 13y ago
adamcavalli 13y ago
typeof(self) __weak weakSelf = self; self.block = ^{ typeof(self) strongSelf = weakSelf; if (strongSelf) { // ... } };
pjungwir 13y ago
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?
- ddustin 13y ago
  This is the correct way to access ivars. Though it makes a lot of indents so I usually do a return. I also use __strong to be explicit to the reader. __strong MyClass *strongSelf = weakSelf; if(!strongSelf) return; ... Rest of block It's also worth noting calling functions on weakSelf can in turn access ivars. Getting a strong reference to self solves both the issues. NSLog calls [self description] in your example.
- deleted 13y ago
  [deleted]
dmishe 13y ago
Nice, just the other day I used NSAssert in C function and got weird crash on self.

90 points by dcaunt 13y ago

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 13y ago
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 13y ago
  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.
- chrisdevereux 13y ago
  Using blocks is the one bit of Objective-C that still reminds me of the old MRR style. Just like with retain/release/autorelease, it's one thing understanding the pattern, it's another ensuring that your entire program conforms to it. Anywhere that you capture nontrivial objects is a possible failure point. ReactiveCocoa has some nice patterns for avoiding block callbacks altogether. Eg: [myTarget rac_liftSelector:@selector(receiveCallback:) withSignals:mySignal, nil]; will capture a weak reference to the target, invoke the callback whenever 1 or more events fire and automatically unsubscribe when the target deallocates.

AshFurrow 13y ago

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.

AshFurrow 13y ago
By the way, you can prevent the retain cycle in NSAssert using libextobjc's @weakify and @strongify macros.
- dcaunt 13y ago
  Indeed, the NSAssert cycle surprised me too. libextobj is great.
deleted 13y ago
[deleted]
seivan 13y ago
Pretty much my take on it as well. Nothing out of the ordinary here except the NSAssert, but I got that before the summer. A hack to get around the self inside the macro https://github.com/seivan/SHAlertViewBlocks/blob/develop/Example/Example/Example-Prefix.pch#L9-L23 https://github.com/seivan/SHAlertViewBlocks/blob/develop/Exa...
stigi 13y ago
I'd recommend looking at @weakify and @strongify that come with libextobjc (https://github.com/jspahrsummers/libextobjc/ https://github.com/jspahrsummers/libextobjc/). Handy little helpers to avoid retain cycles with all kind of blocks.
mackey 13y ago
I agree and disagree. For me, I agree this is standard stuff. If you are writing this stuff everyday, this block song and dance is basically taken care of with muscle memory. Part of my job involves working with people from other companies who aren't as strong with objective-c. There are a lot of people that have trouble blocks for whatever reasons. I recently had a guy from another company act almost surprised that we were using "advanced features like blocks". We were a little shocked. But it just shows that not everyone is comfortable with them. BUT, I think a better approach from the author wouldn't be to tell people not to use blocks w/ NSNotificationCenter but to make sure they know the in and outs of blocks, because blocks are amazingly powerful tools and they aren't going anywhere.

kybernetyk 13y ago

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.

pothibo 13y ago

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 ;)

obiterdictum 13y ago

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

jheriko 13y ago

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 13y ago
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 13y ago
  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.
  - 13y ago

archagon 13y ago

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

Strilanc 13y ago

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-tokens-and-collapsing-futures-for-objective-c http://twistedoakstudios.com/blog/Post7391_cancellation-toke...

13y ago

adamcavalli 13y ago

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

pjungwir 13y ago

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?

ddustin 13y ago
This is the correct way to access ivars. Though it makes a lot of indents so I usually do a return. I also use __strong to be explicit to the reader. __strong MyClass *strongSelf = weakSelf; if(!strongSelf) return; ... Rest of block It's also worth noting calling functions on weakSelf can in turn access ivars. Getting a strong reference to self solves both the issues. NSLog calls [self description] in your example.
deleted 13y ago
[deleted]

dmishe 13y ago

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