Skip to content
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

[SR-5816] KeyPath-based KVO: Unable to stop observing owned object on deinit #3809

Open
antonvmironov mannequin opened this issue Sep 1, 2017 · 7 comments
Open

[SR-5816] KeyPath-based KVO: Unable to stop observing owned object on deinit #3809

antonvmironov mannequin opened this issue Sep 1, 2017 · 7 comments

Comments

@antonvmironov
Copy link
Mannequin

antonvmironov mannequin commented Sep 1, 2017

Previous ID SR-5816
Radar None
Original Reporter @antonvmironov
Type Bug
Environment

Version 9.0 beta 6 (9M214v)

Additional Detail from JIRA
Votes 7
Component/s Foundation
Labels Bug
Assignee None
Priority Medium

md5: cc033b62e7f6826a74055dc7ea71bfeb

Issue Description:

MyHostObject owns MyObject and observes changes of it's value:

import Foundation

class MyObject: NSObject {
  @objc dynamic var value: Int = 0
}

class MyHostObject: NSObject {
  @objc dynamic let myObject = MyObject()
  var _observation: NSKeyValueObservation?

  override init() {
    super.init()
    _observation = observe(\.myObject.value, options: [.new]) { (myObject, change) in
      print("value changed to \(change.newValue!)")
    }
  }

  deinit {
    // `myObject` is in Limbo right now, but I can still call it's methods and, hopefully, remove observation
    _observation?.invalidate()
    _observation = nil
  }
}

var myHostObject: MyHostObject? = MyHostObject()
myHostObject = nil // crashing here with:
// *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'An instance 0x101c10290 of class issue_NSKeyValueObservation.MyHostObject was deallocated while key value observers were still registered with it. Current observation info: <NSKeyValueObservationInfo 0x101c14ed0> (
<NSKeyValueObservance 0x101c14c20: Observer: 0x101c115f0, Key path: myObject.value, Options: <New: YES, Old: NO, Prior: NO> Context: 0x0, Property: 0x101c129b0>
)'

With Objective-C equivalent, MyHostObject would still be able to remove observation from myObject.

This is happening because NSKeyValueObservation holds a weak reference to an object. That weak reference turns to nil too soon. My suggestion is to use unowned or Unmanaged<MyObject> reference.

@belkadan
Copy link

belkadan commented Sep 5, 2017

cc @Catfish-Man

@Catfish-Man
Copy link
Member

Hmm. I wouldn’t expect this to be an issue in iOS 11/macOS 10.13 due to KVO changes, but could be important for backwards compatibility.

@antonvmironov
Copy link
Mannequin Author

antonvmironov mannequin commented Sep 5, 2017

Just tested on iOS 11. No issue there.

@bobergj
Copy link

bobergj commented Oct 10, 2017

Still an issue in macOS 10.12.6 and iOS 10, Swift 4.0 Snapshot 2017-10-09 (a). This makes the new closure-based observe basically unusable for us since we need to be compatible with iOS 10. Only in objects with lifecycle methods (viewDidDissapear and such) can this be workaround by invalidating before dealloc.

@bobergj
Copy link

bobergj commented Oct 10, 2017

Also, according to the Foundation Release Notes for macOS 10.13 and iOS 11 (https://developer.apple.com/library/content/releasenotes/Foundation/RN-Foundation/index.html), the automatic unregistration of observers on dealloc seem to be subject to some conditions (automaticallyNotifiesObserversForKey).

@sinoru
Copy link

sinoru commented Jul 8, 2019

It seems like fixed in Swift 5.1 (iOS 11). But this fix leads app to crash with previous iOS workaround (Manually removing observer in deinit). What should I do?

Error: `Cannot remove an observer <_NSKeyValueObservation 0x28196ef10> for the key path "textField.selected" from <#{SomeClass} 0x10192a480> because it is not registered as an observer.`

@sinoru
Copy link

sinoru commented Sep 5, 2019

Definitely this is fixed by apple/swift#20103

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants