Careful with Fancy KVO Callback

Be warned: there may be a bug in the fancier version of Key Value Observering.

I've been adding some new functionality to my declarative web request library called Decree. In particular, I added the ability to get progress updates on the requests and I'm working on adding the ability to download responses to a file instead of into memory.

While developing, I noticed that the progress unit tests were failing occasionally. Reproducing it, I found that it was crashing when a progress update was attempting to go out. It seemed that an invalid observer was hanging around.

This was the observation code I had:

class ProgressObserver: NSObject {
    @objc dynamic var subject: URLSessionTask
    var observation: NSKeyValueObservation?
    let callbackQueue: DispatchQueue?

    init(
        for subject: URLSessionTask,
        callbackQueue: DispatchQueue?,
        onChange: @escaping (Double) -> ()
        )
    {
        self.subject = subject
        self.callbackQueue = callbackQueue

        super.init()

        self.observation = self.observe(
            \.subject.progress.fractionCompleted,
            options: [.initial, .new],
            changeHandler: { [unowned self] object, change in
                guard let value = change.newValue else {
                    return
                }
                self.callbackQueue.async {
                    onChange(value as Double)
                }
            }
        )
    }
}

This is pretty straightforward and almost an exact copy of Apple's documentation with extra functionality for specifying a callback and callback queue.

The documentation for this observe(:options:chanceHandler) method reads as follows:

when the returned NSKeyValueObservation is deinited or invalidated, it will stop observing

So I had assumed I was safe. Just in case, I tried adding a call to invalidate() in my own deinit. However, I was still getting the crashes.

Next, I tried changing the unowned capture in the closure to a weak one in case that was somehow messing with the lifecycles of the involved objects. Still, I was getting crashes.

Finally, I decided to fallback to the old-school style of observation.

class ProgressObserver: NSObject {
    @objc dynamic var subject: URLSessionTask
    let callbackQueue: DispatchQueue?
    let onChange: (Double) -> ()

    init(
        for subject: URLSessionTask,
        callbackQueue: DispatchQueue?,
        onChange: @escaping (Double) -> ()
        )
    {
        self.subject = subject
        self.callbackQueue = callbackQueue
        self.onChange = onChange

        super.init()

        subject.progress.addObserver(
            self,
            forKeyPath: "fractionCompleted",
            options: [.initial, .new],
            context: nil
        )
    }

    deinit {
        self.subject.progress.removeObserver(
            self,
            forKeyPath: "fractionCompleted"
        )
    }

    override func observeValue(
        forKeyPath keyPath: String?,
        of object: Any?,
        change: [NSKeyValueChangeKey : Any]?,
        context: UnsafeMutableRawPointer?
        )
    {
        guard let value = change?[NSKeyValueChangeKey.newKey] else {
            return
        }
        self.callbackQueue.async {
            self.onChange(value as! Double)
        }
    }
}

This version of observing requires the implementation of the observerValue method that gets called with updates. More importantly, it also explicitly requires manual removal of the observer.

With that change, the intermittent crash went away. It seems there is a bug in the fancier callback version of KVO. Be warned.

Update

After posting this on Reddit another developer told me they've also seen this before.

I’ve seen this before and I found the problem is not the old vs. new-style observation, but the compound (x.y.z) vs simple (z) key path.

I gave it a shot by reverting my code to something very similar to the original with a slight modification.

class ProgressObserver: NSObject {
    @objc dynamic var subject: URLSessionTask
    var observation: NSKeyValueObservation?
    let callbackQueue: DispatchQueue?

    init(
        for subject: URLSessionTask,
        callbackQueue: DispatchQueue?,
        onChange: @escaping (Double) -> ()
        )
    {
        self.subject = subject
        self.callbackQueue = callbackQueue

        super.init()

        self.observation = self.subject.progress.observe(
            \.fractionCompleted,
            options: [.initial, .new],
            changeHandler: { [unowned self] object, change in
                guard let value = change.newValue else {
                    return
                }
                self.callbackQueue.async {
                    onChange(value as Double)
                }
            }
        )
    }
}

Note that I'm calling observe on the progress instance itself and using a single length key path \.fractionCompleted. This also fixed my problem and is cleaner than the old style so I'm going to stick with it.

I realized that I originally wrote it with the extended path because I was still thinking in terms of an observer instance (being my ProgressObserver type), but with the callback observation, that isn't really the paradigm anymore. Instead, we call observe directly on the thing we want to observe.

So, it seems that the API does have a flaw in it, but it's safe to use as long as we call it directly on the object we're observing.