New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make it easier when a strong reference between controller and observe would create a retain loop #67

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@hzwzw

hzwzw commented May 22, 2015

I'm trying to solve #48

Observing self is frequently needed in our project. It will be nice if we don't need to call unobserveAll prior to dealloc.

The reason for the original problem is: A retain loop is created when we set NSMapTable's keys with the NSPointerFunctionsStrongMemory option (when self observes self), but it causes a crash when we set NSMapTable's keys with the NSPointerFunctionsWeakMemory option since there is no chance to remove the keyPath from the observer as a weak property is set to nil in dealloc.

I tried to find a way to set the key option to assign, and I found NSPointerFunctionsOpaqueMemory in Apple's documentation:

Take no action when pointers are deleted.

This is usually the preferred memory option for holding arbitrary pointers.

I have tested this and I'm sure this is what is needed.

Thanks a lot!

@joerick

This comment has been minimized.

joerick commented Jun 30, 2015

I've merged this into master and done some tests on this on my end.

@interface Person : NSObject

@property (strong) NSString *name;

@end

@implementation Person

@end


int main(int argc, const char * argv[]) {
    @autoreleasepool {
        Person *p1 = [Person new];

        // EXAMPLE 1
        {
            Person *p2 = [Person new];
            [p2.KVOControllerNonRetaining observe:p2 keyPath:@"name" options:0 block:^(id observer, id object, NSDictionary *change) {
                NSLog(@"p2 changed!");
            }];
        }

        // EXAMPLE 2
        {
            Person *p3 = [Person new];
            [p1.KVOControllerNonRetaining observe:p3 keyPath:@"name" options:0 block:^(id observer, id object, NSDictionary *change) {
                NSLog(@"p3 changed!");
            }];
        }
    }
    return 0;
}

The change only affects retainObserved:NO, so I'm only testing self.KVOControllerNonRetaining.

Example 1 above is the self-observing-self example. This code crashes before this patch (object dealloc'd while key-value observers registered) and works after.

Example 2 was my concern with this patch, where an object is dealloc'd while being observed, as this would leave bad pointers in the NSMapTable. But this crashes before AND after applying this patch, because Apple's KVO machinery doesn't allow observed objects to be dealloc'd.

It might be worth adding to the docstrings that the user is responsible for unobserving any objects before they are dealloc'd when using KVOControllerNonRetaining?

Regardless, this patch looks good to me! 😄

@hzwzw

This comment has been minimized.

hzwzw commented Jun 30, 2015

@joerick
Thank you very much. I'm so glad that I can help.

It might be worth adding to the docstrings that the user is responsible for unobserving any objects before they are dealloc'd when using KVOControllerNonRetaining?

You don't need to worry about anything if you set NSMapTable's keys with the NSPointerFunctionsOpaqueMemory option.

Because the object of FBKVOController that you have created in your class will unobserveAll in its own dealloc method.

- (void)dealloc
{
  [self unobserveAll];
}
@ghost

This comment has been minimized.

ghost commented Aug 4, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

@caoer

This comment has been minimized.

caoer commented Aug 28, 2015

👍 @hzwzw @joerick
This patch is great.

but for the code below

- (void)dealloc
{
  [self unobserveAll];
}

we can't call KVOControllerNonRetaining's unobserveAll in dealloc, because KVOControllerNonRetaining is lazily initialized. It will crash if it tries to create it in the dealloc method. I've put a simple workaround to use in : caoer@6c4faa8

and a demo code project in: https://github.com/caoer/FBKVOControllerTest/

not sure if there is any better way to do it.

@nlutsenko

This comment has been minimized.

Member

nlutsenko commented Mar 16, 2016

This looks very fragile, since it will not clear out the pointer, and all of the observation info will become stagnant and not going to be removed...
As well as this doesn't solve the problem of removing observers and different order of execution.
Example where this won't work:

  • Deallocate observed object
  • Deallocate controller
  • Controller tries to remove observers, but since observed object is deallocated we are getting into undefined behavior, as we are trying to send message to a pointer in no longer owned memory.

There is a better, but crazy way to do the same thing - discussed in #94 with 2 approaches on implementation. Feel free to reopen this pull request if you still think this is a valuable thing to add and please add more context in that case, or if there is anything I might have missed on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment