iosobjective-cgrand-central-dispatch

Proper use of strongSelf / weakSelf in nested dispatch_after blocks


I have a complicated launch and I need to present and dismiss multiple view controllers. I have this bit of code where I need to use delays in presentation; I was wondering if my use of strongSelf / weakSelf was proper in the following code with these dispatch_after blocks, or if there are improvements that I should make to the code. I know it's outdated Objective-C, I am working with an old, large code base.

__weak typeof(self) weakSelf = self;

dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.1f * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{//CURRENT
    
    __strong typeof(self) strongSelf = weakSelf;
    
    [strongSelf presentViewController:strongSelf.coverVC animated:NO completion:^{
        
        [strongSelf showWindowWithImage:NO];
        
        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.1f * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
            
            __strong typeof(self) strongSelf2 = weakSelf;

            [strongSelf2 dismissViewControllerAnimated:YES completion:^{

                strongSelf2.coverVC = nil;
                [strongSelf2 hideWindow];
            }];
        });
    }];
});

Solution

  • This code is a bit curious, possibly written by someone who did not entirely understand the purpose of the weakSelf pattern, much less the more complicated weakSelf/strongSelf pattern. Admittedly, these can be daunting to new programmers, and when Objective-C programmers first encounter blocks and strong reference cycles, they often start sprinkling weak references everywhere in their code. This looks suspiciously like that.

    So, let’s step back: We use weakSelf pattern in one of two scenarios:

    1. It is possible that self (the view controller) might be dismissed before the dispatch_after fires in 0.1 seconds; or

    2. The use of the non-weak self might result in a strong reference cycle.

    In this case, the first seems exceedingly unlikely (but we’d need to know more about the UI to be positive) and the second doesn’t seem applicable at all, as there is no apparent strong reference cycle risk here.

    Setting that aside, let’s discuss those scenarios where you would further complicate that weakSelf pattern with an additional strongSelf inside it (or two, in this case). We do either when:

    1. We need to use that “weak self” reference in some context where a null value would cause an error; or

    2. You want to protect yourself against race conditions where you are doing steps A and B and “self” might conceivably become null in the intervening period, but that it is important that if you do A that you also do B.

    Again, I am not sure if either of those scenarios apply here. It feels appears that the original programmer was reflexively inserting weakSelf/strongSelf pattern in their asynchronous patterns, possibly not understanding whether it was actually needed or not.


    Bottom line, when dealing with a large legacy codebase, I am not sure I would waste any time on this particular code snippet.

    To my eye, the entire usage of weakSelf looks completely unnecessary, but before you excise all of these weakSelf, strongSelf, and strongSelf2 references, you would want to make sure that (a) there are no strong reference cycle risks (which I don’t see in the code that you’ve shared with us here); and (b) there are no scenarios where the view controller might be dismissed from some other mechanism not shown here.

    But, I do wonder if the above could be reduced to the following:

    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.1f * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
        [self presentViewController:self.coverVC animated:NO completion:^{
            [self showWindowWithImage:NO];
            
            dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.1f * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
                [self dismissViewControllerAnimated:YES completion:^{
                    self.coverVC = nil;
                    [self hideWindow];
                }];
            });
        }];
    });
    

    In short, just because you use a completion handler block, does not mean that you must use weakSelf pattern. We generally only do that when there is an actual strong reference cycle risk. I do not see any such risk here.

    But, if you contemplate this sort of change, you really need to test it to make sure you have not introduced any new strong reference cycles (i.e., use “debug memory graph” debugger feature) and/or contemplated some alternate flows where the view controller was dismissed prematurely. I am not sure all of this development overhead is warranted just to eliminate those three lines of code. And all of this is assuming that you hadn’t simplified the code in the preparation of the example, hiding some other subtle strong reference cycle risk that is not shown in your question.

    When dealing with legacy codebases, you have to weigh the benefit of the change against the development/testing overhead associated with that change. Personally, while the code screams “over-complicated” to me, I might ask whether this is the best use of your resources to validate the change.