sphero-inc / Sphero-iOS-SDK

🚫 DEPRECATED: Sphero™ is the amazing robotic ball ( sphero.com ) created by Orbotix, this is the repository for the iOS SDK for Sphero™. Visit dev site for more information:
http://sdk.sphero.com
225 stars 81 forks source link

-[RKConvenienceRobot addResponseObserver:] retains the observer. #22

Closed sodastsai closed 9 years ago

sodastsai commented 9 years ago

I think this is an enhancement proposal.

The story is that ... I have a class, named SpheroDevice, which has a strong property to the RKConvenienceRobot object. And also it implements the RKResponseObserver protocol and is a response observer of the robot it owns.

Like the following code:

@interface SpheroDevice : NSObject <RKResponseObserver>

@property (nonatomic, strong) RKConvenienceRobot *robot;

@end

@implementation SpheroDevice

- (instancetype)init {
    if (self = [super init]) {
        // ... setup sphero connection
    }
    return self;
}

- (void)dealloc {
    [self.robot disconnect];
}

- (void)handleRobotStateChangeNotification:(RKRobotChangedStateNotification *)notification {
    switch (notification.type) {
        case RKRobotOnline: {
            // ... other setup code that assigns a RKConvenienceRobot to self.robot
            [self.robot addResponseObserver:self];
            break;
        }
        case RKRobotDisconnected: {
            [self.robot removeResponseObserver:self];
            // ... other setup code that clean up self.robot property
            break;
        }
        // other cases ...
        default:
            break;
    }
}

- (void)dealloc {
    [self.robot removeResponseObserver:self];
}

So you may find that the retain cycle is: SpheroDevice --(via robot property)--> RKConvenienceRobot --(via response observers)--> SpheroDevice, and thus the object is never freed. (cuz I disconnect it only when it's being freed. and the -[RKConvenienceRobot removeResponseObserver:] only gets called when the SpheroDevice object being disconnected. but the object is freed only when it's not response observer.)

Anyway, I have my workaround to solve this situation. (Send a -[RKConvenienceRobot disconnect] call in other state of my app but not only in object deallocation)

So I suggest that when providing the documentation, you should notify developers that this method will retain the observer and the another one (removeResponseObserver) will release it.

Another suggestion is do not retain the observer. Use a weak reference instead. (Use __weak variable, NSHashTable (weak reference alternative of NSSet), NSMapTable (weak reference alternative of NSDictionary) or other weak ref. collections) Of course you could use __unsafe_unretained or assign properties, but they are not safe that if developers forget to remove from the RKConvenienceRobot before the response observer being freed, the app will get an EXC_BAD_ACCESS exception when the robot wanna send a response to the observer. (But some of Apple's API does so, like the KVO. i.e. -[NSObject addObserver:forKeyPath:options:context:])

sodastsai commented 9 years ago

Btw, if you want to check the retain count of an object under ARC, like the self object, you could use following call:

NSLog(@"%ld", (long)CFGetRetainCount((__bridge CFTypeRef)self));
iamcgn commented 9 years ago

Thanks again for the comments! A fix for this problem is already on our radar. I'll update and close this post when a release is available with the fix.

sodastsai commented 9 years ago

Hi,

I also noticed that sometimes the -[RKConvenienceRobot removeResponseObserver:] doesn't release the observer well.

NSUInteger x = (NSUInteger)CFGetRetainCount((__bridge CFTypeRef)self);
[self.robot removeResponseObserver:self];
NSUInteger y = (NSUInteger)CFGetRetainCount((__bridge CFTypeRef)self);
if (x == y) {
    NSLog(@"Broken!");
}

It doesn't always happen, but I met this couple of times.

zenelk commented 9 years ago

Just did another update on top of Beta 3 to fix this. Please let me know if there is any more issues.