[SR-5117] KeyPath-based KVO: Observation objects deallocated immediately

[zwaldowski]

Previous ID	SR-5117
Radar	rdar://problem/32593788
Original Reporter	@zwaldowski
Type	Bug

Environment

Xcode Version 9.0 beta (9M136h) / swiftlang 900.0.43

Additional Detail from JIRA

| | | |------------------|-----------------| |Votes | 0 | |Component/s | | |Labels | Bug | |Assignee | None | |Priority | Medium | md5: eea4212f54b3fc3022ffa90a1a812368

Issue Description:

Based on the usage pattern from WWDC session 201, consider the following:

import UIKit

class MyObject: NSObject {
    @objc dynamic var foo: Int = 0
    @objc dynamic var bar: String = ""
    @objc dynamic var baz: NSAttributedStringKey?
}

let x = MyObject()
x.observe(\.foo) { (x, y) in print(x, y) }
x.observe(\.bar) { (x, y) in print(x, y) }
x.observe(\.baz) { (x, y) in print(x, y) }
x.foo = 5
x.bar = "blah"
x.baz = .link
x.foo = 6

The session and the (currently nonexistent) documentation do not indicate the observations need to survive. In an application target, the observation objects are immediately deallocated and invalidated. Binding them to a name makes them survive the scope. In a playground, they will work as expected.

[jckarter]

@swift-ci create

[jckarter]

Do you at least get the "unused result" warning? We could tailor the warning in this specific case to be more descriptive.

[mattneub]

@jckarter There is indeed an unused result warning. But what are we supposed to do with the captured result? As the OP says, this is all undocumented. My guess is that if our desire is to observe once, we are supposed to `invalidate`. But this code is illegal:

class C1 : NSObject {
    let avpvc = AVPlayerViewController()
    func test() {
        let obs = self.avpvc.observe(\.readyForDisplay, options:[.new]) { (obj, ch) in
            if let ok = ch.newValue, ok {
                // do work here ...
                obs.invalidate() // compile error
            }
        }
    }
}

In other words, if the NSKeyValueObservation object is worth the warning (i.e. worth capturing), shouldn't it be usable for something?

Do let me know if I'm just confused; this is all undocumented and there are no headers available to me in Xcode beta 3, so I have to guess at your intentions.

[jckarter]

AIUI, the observation gets unregistered when the returned object is destroyed, so you're intended to hold onto it for as long as you need the observation, say in an instance property of the observing object.

[mattneub]

@jckarter But then isn't the unused result warning inappropriate?

[jckarter]

Not sure what you mean. If you don't use the result object, then the observation is going to be ended immediately. If you use the result by storing it somewhere, you won't get the warning.

[mattneub]

@jckarter You're not the right person to complain to. Sorry about that.

The problem is apparently that they've implemented this whole API incorrectly. The case in point is that I want to observe once and then lose the observer (actually quite common in the world of KVO). Then this (just to give a simple test example) does work, and I've managed to avoid getting a warning from Swift, but what I'm forced to do is just silly:

func delay(_ delay:Double, closure:@escaping ()->()) {
    let when = DispatchTime.now() + delay
    DispatchQueue.main.asyncAfter(deadline: when, execute: closure)
}

class A : NSObject {
    @objc dynamic var name = "Matt"
}

class ViewController: UIViewController {
    override func viewDidLoad() {
        super.viewDidLoad()
        let a = A()
        let ob = a.observe(\.name) { obj, ch in
            print(obj.name)
        }
        delay(2) {
            _ = ob // silly!
            print("here we go")
            a.name = "Joe"
        }
    }
}

You see what's wrong with that? The preservation of `ob` is in the wrong place. But I can't put it inside the `observe` closure — the Swift compiler will stop me. And I can't initialize `ob` as a `var` beforehand, because the compiler won't let me initialize an NSKeyValueObservation:

        var ob = NSKeyValueObservation() // compile error
        ob = a.observe(\.name) { obj, ch in

Contrast what they've done here with how you do the same sort of thing on the NotificationCenter:

        var ob1 : NSObjectProtocol = NSObject()
        ob1 = NotificationCenter.default.addObserver(forName: .NSMetadataQueryDidStartGathering, object: nil, queue: queue) { _ in
            NotificationCenter.default.removeObserver(ob1)
            // ...
        }

That's a very standard pattern, and there's a standard way to do it. But they've blocked that way of doing it with this observer object, because I can't find a way to refer to the observer object inside the closure.

[mattneub]

Okay, I've found a workaround. This does what I want to do:

func delay(_ delay:Double, closure:@escaping ()->()) {
    let when = DispatchTime.now() + delay
    DispatchQueue.main.asyncAfter(deadline: when, execute: closure)
}

class A : NSObject {
    @objc dynamic var name = "Matt"
}

class ViewController: UIViewController {
    override func viewDidLoad() {
        super.viewDidLoad()
        let a = A()
        var ob = NSObject()
        ob = a.observe(\.name) { obj, ch in
            print(obj.name)
            (ob as! NSKeyValueObservation).invalidate()
        }
        delay(2) {
            print("here we go")
            a.name = "Joe"
        }
        delay(4) {
            print("here we go again")
            a.name = "Joe"
        }

    }
}

As the printout shows, we observe long enough to get the first notification and then no more. Having to type the observer as an NSObject is skanky but it avoids my being stopped by the Swift compiler. Of course I'd prefer if the compiler would just let me initialize an NSKeyValueObservation.

[jckarter]

Foundation owns this API, so if you have feedback about how it works, Radar might be a better place for it. For these two issues, using an optional variable seems like it would address both of them:

var ob: NSKeyValueObservation? // defaults to nil
...
ob = a.observe(...)
...
async {
  ...
  ob = nil
}