CODEWITHSUNDEEP
iosobjective-cautomatic-ref-countingobjective-c-blocksretain-cycle
gadu
gadu

Reputation: 1826

Retain cycle warning on __block variable that is an ivar

I'm subclassing AVQueuePlayer, and in my constructor, where I pass the AVPlayerItem's it needs to play, I want to add an observer on the first item to play.

So I'm using the AVPlayer method addBoundaryTimeObserverForTimes:queue:usingBlock:. Proper implementation requires me to call removeTimeObserver: on the "opaque" object that addBoundary method returns.

In order to retain the object for however long is necessary, I declared it as a __block ivar:

@property (nonatomic, copy) __block id obs;

then in my own init method I have: 

__block AVPlayer* blockPlayer = self;

_obs = [self addBoundaryTimeObserverForTimes: times
             queue:NULL
             usingBlock:^{ 

                 // Post a notification that I can then act on
             [[NSNotificationCenter defaultCenter]
                      postNotificationName:@"PlaybackStartedNotification"
                      object:nil];

                  // Remove the boundary time observer
             [blockPlayer removeTimeObserver:_obs]; // Warning here
        }];

There's a lot going on here... And although the specific warning comes when I try to remove the time observer, I'm also posting a notification, which I may also change to pass a variable in the object: part. I'm also setting self as the observer...

I've read a lot of other answers on potential solutions (example) but I haven't really found anything about using block variables.

Is my code unsafe or am I okay?

Edit: I originally mistyped the name of the @property as __block id observer when I indeed meant for it to be __block id obs. The accepted answer thus answered both scenarios! (Awesome!)

Upvotes: 4

Views: 521

Answers (1)

CRD
CRD

Reputation: 53000

(All code typed directly into answer, treat it as pseudo-code and expect there to be minor typos at least!)

You are unfortunately misunderstanding the purpose and behaviour of __block - it is an attribute which only applies to local variables and modifies their lifetime so they can be safely updated by a block. So:

In order to retain the object for however long is necessary, I declared it as a __block ivar:

@property (nonatomic, copy) __block id observer;

is an invalid property declaration, as properties are instance methods usually backed by instance - not local - variables. The current Apple compiler unfortunately just ignores the meaningless __block rather than reporting an error - a bug which has previously caused confusion for SO inquirers (you should submit a bug report to Apple to encourage them to fix it).

Next you write:

__block AVPlayer* blockPlayer = self;

so you can use blockPlayer within your block instead of self in an attempt to avoid a retain cycle. This does not work, indeed it simple adds another (anonymous) object into the cycle... What you need here is a weak reference:

__weak AVPlayer *blockPlayer = self;

Weak references break a cycle, but within the block you must first create a strong reference from them and check it is not NULL - which it will be if the object it weakly references has been destroyed. The code to do this within your block will be something like:

// Remove the boundary time observer if blockPlayer still exists
AVPlayer *strongBlockPlayer = blockPlayer; // obtain strong reference from weak one
if (strongBlockPlayer)
    [strongBlockPlayer ...];

Even after these changes you have a bigger problem. In your code you have:

_obs = [self addBoundaryTimeObserverForTimes:times
                                       queue:NULL
                                  usingBlock:^{ 
          ...
          [blockPlayer removeTimeObserver:_obs];
        }];

Here you are attempting to use the value of _obs within your block.

At this point your question becomes unclear, is _obs meant to be a local variable or the property observer? We'll consider both cases:

Local Variable

If _obs is a local variable your code will not work. When a block is created the values of any local variables used by the block are copied into the block itself, so here the value of _obs would be copied. However _obs will not have a valid value at this point, that will only happen after the call to addBoundaryTimeObserverForTimes:queue:usingBlock: returns and the assignment of its return value, which is after the block has been created and passed to the same call...

This problem is similar to defining a local recursive block and has the same solution. If a local variable is declared with the __block attribute, so that its lifetime is modified to match that of any blocks using it and thus those blocks can modify its value, then the value in the local variable is not copied into the block - instead the block gets a reference to the variable which it uses to read/write the variable as needed.

So to make the code work you change it to:

__block id obs = [self addBoundaryTimeObserverForTimes:times
                                                 queue:NULL
                                            usingBlock:^{ 
                    ...
                    [blockPlayer removeTimeObserver:obs];
                  }];

In this version:

  1. The local variable obs will be created in such a way that its lifetime is at least as long as your block (we'll skip the details of how the compiler arranges this - they are interesting but not of critical importance);
  2. The block is created with a reference to the variable jobs;
  3. The method addBoundaryTimeObserverForTimes:queue:usingBlock: is called passing it the block;
  4. The method returns and its result is assigned to obs; and
  5. (A little later) the block is invoked, reads obs, and obtains the value stored there after the method call.

Property Reference

If you've made a typo and you intended _obs to be the property observer then the LHS of the assignment should be self.observer and the RHS should be blockPlayer.observer, which allowing for the need for the weak reference would be:

__weak AVPlayer *blockPlayer = self;
self.observer = [self addBoundaryTimeObserverForTimes:times
                                                queue:NULL
                                           usingBlock:^{ 
                   ...
                   // Remove the boundary time observer if blockPlayer still exists
                   AVPlayer *strongBlockPlayer = blockPlayer; // obtain strong reference from weak one
                   if (strongBlockPlayer)
                      [strongBlockPlayer removeTimeObserver:strongBlockPlayer.observer];
                 }];

This will work as by the time the block is called and reads strongBlockPlayer.observer the call to addBoundaryTimeObserverForTimes:queue:usingBlock: block will have returned and the assignment to the property made.

Local Variable vs. Property for obs/observer?

Which of the above two versions, both of which should work, is better? Probably the local variable version as (a) you don't appear to need the property elsewhere and (b) it localises the need for the variable to just the statement, the method call, which needs it, which in turn helps readability and debugging. However that is an opinion and some might disagree - make your own choice!

HTH

Upvotes: 2

Related Questions