1

I'm trying to understand what is wrong with the following code.

Imagine the following class, "Foo":

@protocol FooDelegate <NSObject>
- (void)hereTakeThisFooBarDic:(NSDictionary *)fooBarDic;
@end

@interface Foo : NSObject <BarDelegate>
@property (nonatomic, strong) Bar *bar;
@property (nonatomic, weak) id <FooDelegate> *fooDelegate;
- (void)getFooBarDicForNum:(int)fooBarNum;
@end

@implementation Foo

static Foo *foo = nil;

- (id)init {
    if (!foo) {
        foo = [super init];
        self.bar = [[bar alloc] init];
    }
    return foo;
}

- (void)getFooBarDicForNum:(int)fooBarNum {
    self.bar.fooDelegate = self;
    [self.bar getFooBarDicFromIntarwebsNumber:fooBarNum];
}

//We get this callback from self.bar after a few ms
- (void)callbackWithFooBarDicFromIntarwebs:(NSDictionary *)fooBarDic {
    [self.fooDelegate hereTakeThisFooBarDic:fooBarDic];
}
@end

We call Foo from somewhere in code like this:

   for(int i=0; i < 10; i++) {
       Foo *foo = [[Foo alloc] init];
       [foo getFooBarDicForNum:i];
   }

Then we get the callbacks later in a hereTakeThisFooBarDic method.

But the problem is we are getting unbounded memory growth. It seems that Foo's init method acts like a singleton, but every time we call it we are allocating more memory for it. It does not register as a memory leak though. In looking at this code it does not seem like the right way to do a singleton, though.

I'd like to know what the authors of this code did wrong.

CommaToast
  • 11,370
  • 7
  • 54
  • 69

2 Answers2

2

Using ARC the init method you show should not leak, and at least with Xcode 7.2/Clang 7.0.2 does not leak (i.e. the compiler correctly implements the ARC semantics).

However the init method is not "correct" (ignoring any multithreading issues), even though it works in this case:

- (id)init {
    if (!foo) {
        foo = [super init];

The above line appears to assume that the value return by [super init] will be the same as self as in the next line...

        self.bar = [[bar alloc] init];

It assigns to self assuming this is the same value as in foo...

    }
    return foo;

And such assumptions are not only wrong, but this very init method is an example of one which may not return the self it was passed!

}

At least you didn't write it!

What the author should have written (if following this particular "shared instance" pattern) is something along the lines of:

- (id)init
{
   if (!foo)
   {
      self = [super init];
      self.bar = [[Bar alloc] init];
      foo = self;
      return self;
   }

   return foo;
}

That said, this is not the recommended shared instance pattern. If you are interested in that and how to make a modern singleton pattern there are plenty of references, I'll point you at this one, can't think why ;-)

As to your suspected leak, it's not the init and does not appear to be the other two methods either, so you'll need to look at what those in turn call. Happy hunting!

HTH

Community
  • 1
  • 1
CRD
  • 52,522
  • 5
  • 70
  • 86
  • Yeah the one you link to is how I always make my singletons. I wasn't sure if this other was was advisable.. and as you alluded, it certainly does not seem threadsafe. And yeah, the guys who wrote this? They were using it to receive the callbacks of NSURLSession... facepalm... – CommaToast Mar 02 '16 at 04:09
  • Further reading: ARC's behavior here is described by the ["ns_consumes_self"](http://clang.llvm.org/docs/AutomaticReferenceCounting.html#consumed-parameters) annotation, implicitly applied to `init`. See also [In ARC, why is self in init methods a consumed parameter?](http://stackoverflow.com/q/17242328). @CommaToast – jscs Mar 03 '16 at 20:09
  • @JoshCaswell - exactly, though the referenced question I think misreads the semantics slightly - the point is ownership is transferred from caller to callee, a caller-side retain is only required if the caller doesn't already have ownership. I didn't write my answer though until I'd checked the current compiler was implementing these semantics correctly, just in case... – CRD Mar 03 '16 at 21:15
1

Every time you call [[Foo alloc] init], a new Foo is being created, but ignored. If you're not compiling with ARC, this is indeed a leak: you've allocated something and are not releasing it. If you are, it's still weird and I'm not sure it would surprise me if it was still a leak.

If you want a singleton that operates this way, you must allocate it only once, and ensure that subsequent calls to +alloc return that same instance. Peter Hosey has a very interesting blog post about it.

jscs
  • 63,694
  • 13
  • 151
  • 195
  • Blog seems to be down at the moment, but the code is available for perusal: https://bitbucket.org/boredzo/singleton/src – jscs Mar 01 '16 at 03:40
  • The weird thing is that if I do: `Foo *foo1 = [[Foo alloc] init];` followed by `Foo *foo2 = [[Foo alloc] init];` then they return identical pointers (i.e. `foo1 == foo2` is true). So are you convinced that the alloc method is indeed leaking? Instruments doesn't report it as a leak... is it possible that a new memory page is being set aside and just never gets used for anything? Its very confusing. – CommaToast Mar 01 '16 at 04:42
  • Also that singleton link you sent me looks very pre-ARC. When I write singletons I use the dispatch method typically. But this code came from overseas... – CommaToast Mar 01 '16 at 04:44
  • They return identical pointers because `init` is returning `foo` every time. It gets assigned the first time `init` runs, and then returned after that. `alloc` is the method responsible for allocating memory for the new instance. That instance is passed into `init` (as `self`) and no other reference to it exists. It absolutely leaks without ARC. – jscs Mar 01 '16 at 04:57
  • Peter's code is easy enough to update to ARC, but the standard `dispatch_once()` idiom is generally fine. I just pointed to the blog for further understanding. – jscs Mar 01 '16 at 04:59
  • But I'm using ARC, so would alloc still leak or cause unbounded memory growth in the above code? – CommaToast Mar 01 '16 at 15:40
  • I would not be astonished if it did. Are you seeing a leak? – jscs Mar 01 '16 at 21:02
  • Unbounded growth. But not sure from what exactly... as tends to be the case with unbounded growth if you know what i mean. – CommaToast Mar 02 '16 at 04:10