Leaderboard Requests, Nested Blocks, and Retain Cycles

Question

I have developed a leaderboard display class for my iPhone game. The class has the following instance method.

-(void)displayScoresWithRequest:(CBLeaderboard*)request completionHandler:(void(^)())completionHandler
{
    if (request_ != nil)
        return;

    request_ = [[CBLeaderboard alloc] init];
    [request_ setCategory:[request category]];
    [request_ setPlayerScope:[request playerScope]];
    [request_ setTimeScope:[request timeScope]];
    [request_ setRange:[request range]];

    __block CBLeaderboardDisplay* blockSelf = self;
    [request_ loadScoresWithCompletionHandler:^(NSArray* scores, NSError* error)
    {
        blockSelf->request_ = nil;

        NSUInteger scoresCount = [scores count];
        if (scoresCount == 0 && error != nil)
            return;

        NSMutableArray* playerIDs = [NSMutableArray array];
        for (GKScore* score in scores)
            [playerIDs addObject:[score playerID]];

        [GKPlayer loadPlayersForIdentifiers:playerIDs withCompletionHandler:^(NSArray* players, NSError* error)
        {
            if (scoresCount > [players count] && error != nil)
                return;

            [blockSelf displayScores:scores players:players];

            completionHandler();
        }];
    }];


    [request_ release];
}

As you can see, the method copies a leaderboard request, executes it, and calls the supplied completion handler. A layer in my game calls this method as follows.

-(void)refreshDisplay
{
    CBLeaderboard* request = [[CBLeaderboard alloc] init];
    [request setCategory:[[sharedGameCenterManager_ classicLeaderboard] category]];
    [request setPlayerScope:GKLeaderboardPlayerScopeFriendsOnly];
    [request setTimeScope:GKLeaderboardTimeScopeAllTime];

    static NSRange kRequestRange = NSMakeRange(1, 3);
    [request setRange:kRequestRange];

    __block GJGameOver* blockSelf = self;
    [display_ displayScoresWithRequest:request completionHandler:^
    {
        CGSize displayContentSize = [blockSelf->display_ contentSize];
        displayContentSize.width = width(blockSelf) - 2.0 * kGJLabelPadding;
        [blockSelf->display_ setContentSize:displayContentSize];

        CGFloat displayHeight =
            bottomEdge(blockSelf->multiplierLabel_) - topEdge(blockSelf->menu_) - 2.0 * kGJLabelPadding;
        CGFloat displayScoreDisplaysCount = [blockSelf->display_ scoreDisplaysCount];
        CGFloat displayLabelPadding =
            (displayHeight - [blockSelf->display_ minContentSize].height) / displayScoreDisplaysCount;
        [blockSelf->display_ setLabelPadding:MIN(floor(displayLabelPadding), kGJLabelPadding)];

        static CGFloat kFadeInDuration = 2.0;
        if ([blockSelf->display_ opacity] == 0)
            [blockSelf->display_ runAction:[CCFadeIn actionWithDuration:kFadeInDuration]];
    }];

    [request release];
}

My game crashes when both the layer and hence display are deallocated and the request has not completed. When the request completes, it attempts to send a message to a deallocated instance and the crash ensues. Is it possible to cancel a leaderboard request? If not, is there any way I can avoid the crash without causing a memory leak?

You need __weak in addition to __block in an ARC context to eliminate retain cycles. Also, none of this is explicitly threadsafe, which is what GameKit requires of callbacks. — CodaFi, Mar 12 '13 at 05:29
Oh dear. Well, then, this could get a little hairy. Your thread, and consequently the block need a copy of self to keep around so that none of this premature deallocation stuff happens. I would get rid of the block qualifier, and let the block retain a copy of self, as weird as that sounds. — CodaFi, Mar 12 '13 at 05:36
@CodaFi Thanks. I thought about that myself. Hopefully the dealloc will still occur. I'll have to debug and verify. — Sam Hertz, Mar 12 '13 at 05:41
Or you could broadcast a notification. No more retain cycles, and if self has gone the way of the dinosaurs, it will be a No-Op (so long as you de-register in dealloc) — CodaFi, Mar 12 '13 at 05:44
@CoadFi Can you elaborate? I can think of a solution that sets some global variable on dealloc which the block can then read, but such a solution is ugly to say the least :D — Sam Hertz, Mar 12 '13 at 06:29
I'm saying you could broadcast an NSNotification to the class instead of explicitly using self and creating retain cycles. Then, it doesn't matter whether or not the object exists, it matters whether the object is subscribed to a notification, and that's far more controllable. — CodaFi, Mar 12 '13 at 06:33
For now I have fixed the problem by checking if the leaderboard display has a parent in its completion handler. Either way, there is a more fundamental problem here. These requests are made every time the game over screen appears. Since the player could potentially cycle through the game play and game over screen many times, there could be many requests that remain unserved (I've noticed that Game Center likes to try as long as there is connectivity). It seems I may need to delegate the requests to a singleton that can limit the total number of them. — Sam Hertz, Mar 12 '13 at 08:00

score 0 · Answer 1 · answered Mar 13 '13 at 05:22

In both of your blocks, you use __block to allow the block to reference self without retaining it. This is the problem, because you are doing an asynchronous operation, and if the block is executed after self has been deallocated, it is using a dangling pointer. The whole point of blocks retaining objects they capture is to keep them alive so the block can use it.

Not retaining self when making blocks is commonly done to avoid retain cycles. However, I don't see any retain cycles here:

The request_ in displayScoresWithRequest probably retains the block in displayScoresWithRequest
The block in displayScoresWithRequest retains self, the CBLeaderboardDisplay object
The block in displayScoresWithRequest retains the block from refreshDisplay
The block in refreshDisplay retains self, the GJGameOver object
The GJGameOver object retains display_, the CBLeaderboardDisplay object

However, the CBLeaderboardDisplay object does not retain its instance variable request_. (This code is extremely poorly written, as request_ is released at the end of the method but not set to nil. It should probably be made a local variable or something. And a boolean flag should be used if you want to check whether the code has run once or not.)

I agree, there would not be any retain cycles if each block were allowed to retain its respective self. However, I am mostly concerned about a player that repeatedly reaches the game over screen. This would result in many requests that could go unanswered (I've seen this happen while debugging). I solved this problem by delegating requests to an object that persists in memory and broadcasting notifications. I think the phrase "poorly written" is a little harsh. I am simply using `request_` to indicate if an executing request already exists. I have since changed this variable to a BOOL. — Sam Hertz, Mar 13 '13 at 14:33

Leaderboard Requests, Nested Blocks, and Retain Cycles

1 Answers1