Questions/Possible issues with Advanced Operations sample code

The Advanced NSOperations (https://developer.apple.com/videos/wwdc/2015/?id=226) talk from this year was a real eye opener. I've used NSOperations in my app before but not to the extent as shown in Advanced NSOperations.


I've spent a little time to translate some of the sample code from the Advanced NSOperations for my app and have started using the custom operations and queues to implement CloudKit syncing.


I also took some time to read the Concurrency Programming Guide as well as the docs for NSOperation and NSOperationQueue. As I did I made some notes about things in the docs that didn't seem to match up with the sample code implementation. I'm listing them here just in case they come to anyone else:


In Operation.swift, the isReady method is overwritten to support custom conditions. If the Operation is .Pending, and the super's isReady returns true, then the conditions are evaluated. If the state of the Operation is .Ready, then the operation isReady returns true if the super's isReady returns true. Here is the implementation:


override var ready: Bool {
    switch state {
        case .Pending:
            if super.ready {
                evaluateConditions()
            }
       
            return false
   
        case .Ready:
            return super.ready
   
        default:
            return false
    }
}


In the NSOperation docs however, it states (in regards to overridding the isReady propery):


"In OS X v10.6 and later, if you cancel an operation while it is waiting on the completion of one or more dependent operations, those dependencies are thereafter ignored and the value of this property is updated to reflect that it is now ready to run. This behavior gives an operation queue the chance to flush cancelled operations out of its queue more quickly."


Based on this, it would seem that if the operation is in it's .Cancelled state, isReady should return true, thus allowing giving the queue the chance to flush this operation out of the queue.


Following along the early cancellation thread, in Operation.swift the `-start` method is overridden to look like this:


override final func start() {
    assert(state == .Ready, "This operation must be performed on an operation queue.")
    state = .Executing
   
    for observer in observers {
        observer.operationDidStart(self)
    }
   
    execute()
}


That assert above means that reaching this method in anything other than the .Ready state is a programmer error. But according to the NSOperation docs on -start, this isn't the expected behavior:


"The default implementation of this method updates the execution state of the operation and calls the receiver’s main method. This method also performs several checks to ensure that the operation can actually run. For example, if the receiver was cancelled or is already finished, this method simply returns without calling main. (In OS X v10.5, this method throws an exception if the operation is already finished.) If the operation is currently executing or is not ready to execute, this method throws an

NSInvalidArgumentException
exception. In OS X v10.5, this method catches and ignores any exceptions thrown by your main method automatically. In OS X v10.6 and later, exceptions are allowed to propagate beyond this method. You should never allow exceptions to propagate out of your main method."


Should there be a check here to see if the state is .Cancelled and thus skip calling executing and just go right to .Finished or .Finishing?


That is all I have for now.


Thanks again to the team for this great talk and releasing this sample code to the world.

Replies

Yes, this is a couple of subtle bugs that are in the Advanced NSOperations sample. I hope to be putting out an updated version of the code soon with as many of these fixed as I can.


Here are some of the problems I've identified:

  • Operation overrides start(); it should be overriding main() instead
  • Cancellation is not an explicit state in the state machine. Rather, it should be a flag that gets OR'd in to every state.
  • Accessing the Operation.state property is not thread-safe
  • Operations inside a GroupOperation may evaluate their conditions at unexpected times

LocationOperation bug => func locationManager swift:69:10: Objective-C method 'locationManager:didUpdateLocations:' provided by method 'locationManager(_:didUpdateLocations:)' conflicts with optional requirement method 'locationManager(_:didUpdateLocations:)' in protocol 'CLLocationManagerDelegate'


After you change parameter to _didUpdateLocations the func never gets executed

@davedelong: will you respond to this thread with an announcement when the code is updated along with download instructions - or to point to another thread which can be followed for the same purpose? Unfortunately, the sample code no longer even compiles under Xcode 7, Beta 3.


Advanced NSOperations was, by far, the most interesting session at WWDC2015. I had never considered using Operations to encode non-trivial state machines, even though it is obvious after watching.

  • Change the Bundle Identifier in the General Tab of the Advanced NSOperations project
  • Change the following:
    override func observeValueForKeyPath(keyPath: String?, ofObject object: AnyObject?, change: [NSObject : AnyObject]?, context: UnsafeMutablePointer<Void>) {


to


    override func observeValueForKeyPath(keyPath: String?, ofObject object: AnyObject?, change: [String : AnyObject]?, context: UnsafeMutablePointer<Void>) {


Change the following:

    func locationManager(manager: CLLocationManager, didUpdateLocations locations: [AnyObject]) {
        if let locations = locations as? [CLLocation], location = locations.last where location.horizontalAccuracy <=


to


    func locationManager(manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) {
        if let location = locations.last where location.horizontalAccuracy <= accuracy {


Change the following in NetworkObserver.swift:

/
class Timer {
    /
    private var isCancelled = false

    /
    init(interval: NSTimeInterval, handler: dispatch_block_t) {
        let when = dispatch_time(DISPATCH_TIME_NOW, Int64(interval * Double(NSEC_PER_SEC)))
  
        dispatch_after(when, dispatch_get_main_queue()) { [weak self] in
            if self?.isCancelled == true {
                handler()
            }
        }
    }

    func cancel() {
        isCancelled = true
    }
}


to

class Timer {
    /
    private var isCancelled = false

    /
    init(interval: NSTimeInterval, handler: dispatch_block_t) {
        let when = dispatch_time(DISPATCH_TIME_NOW, Int64(interval * Double(NSEC_PER_SEC)))
   
        dispatch_after(when, dispatch_get_main_queue()) { [weak self] in
            if self?.isCancelled == false {
                handler()
            }
        }
    }

    func cancel() {
        isCancelled = true
    }
}


And in NegatedCondition, change:

            if result.error == nil {


to


            if result.error != nil {


These changes should address issues that I have found in making the code work for the latest Beta 3 release. Note that in the prior posting Dave mentions a few more errors that he'll be addressing:

  • Operation overrides start(); it should be overriding main() instead
  • Cancellation is not an explicit state in the state machine. Rather, it should be a flag that gets OR'd in to every state.
  • Accessing the Operation.state property is not thread-safe
  • Operations inside a GroupOperation may evaluate their conditions at unexpected times

Note too that I was unable to run the Earthquakes App under Xcode 7 on a device, though it worked fine in the simulator and fine when the device was untethered.

Once I changed the Operation to override main() and not start(), the problem seemed to be solved!

Note that I never could run the Earthquakes App under Xcode 7 on a device, though it worked fine in the simulator and fine when the device was untethered.


Once I changed the Operation to override main() and not start(), the problem seemed to be solved! Looking forward to your other updates in a future release.

You may want to read up on the purposes of NSOperation's start and main methods. The Operation class in this case shouldn't be overriding main(). The default behavior of NSOperation is to flag as complete whenever main() finishes.The start() method calls main(). This is why start() is called and KVO is used to indicate the completion of an operation.

Dave,


any news on an updated version of the earthquake sample?


In regards to overriding `start()` vs. `main()`: If one overrides `main()` there is no possibility to do asynchronous/concurrent tasks. See PerUa's comment.


Best regards, Klaas.

Dave,


Any updates? I have disocvered that the reachability condition (or any condtion) in the download task puts the group operation in a stuck state (will never complete if the condition fails). I have been attempting to fix this issue but have yet not been able to track down the root cause. Any help at all would be much appreacated.

A follow up on this :


Any GroupOperation that is canceled does not propagate this up the chain , so the parent operation will never be canceled or finish. By design or bug?

Ok observation 2 :

I can make it work by doing the following :

in every group operation in the chain have :

    override func operationDidFinish(operation: NSOperation, withErrors errors: [NSError]) {
  
        if operation.cancelled {
          
            self.cancel()
        }
      
    }

And the operation child operation can not be canceled until all operations are already init and running, which solves problem 2: conditions are evaluated before operation is executed.


This is handled by wraping the init for the operation group in a simple block operation. There has to be a better way, any thoughts?

As of Xcode Beta 4, I'm getting this error with the AlertModalOperation and the MoreInformationOperation:


Attempting to load the view of a view controller while it is deallocating is not allowed and may result in undefined behavior (<UIAlertController: 0x7fbf3780b530>)

Anyone else getting this behavior or know where the fix is? Dug around and tried to change some of the presentationContext logic but no success.

Yes, when you implement asynchronous subclass `NSOperation` you always override `start`. As the documentation for `start` says: "If you are implementing a concurrent operation, you must override this method and use it to initiate your operation."


Having said that, it's very common to override `main`, too. As the documentation for `main` says: "If you are implementing a concurrent operation, you are not required to override this method but may do so if you plan to call it from your custom

start
method."


Personally, when doing asynchronous `NSOperation` subclasses, I always leave `start` with the boring "setup a basic asynchronous `NSOperation` subclass" stuff and then have it call `main`, and then in `main` I put my app code (e.g. starting the network request, initiating some asynchronous task, etc.).

Have you tried to use the NoCancelledDependencies condition that is part of the sample project? Technically, that is exactly what it's designed to handle:


NoCancelledDependencies - A condition that specifies that every dependency must have succeeded. If any dependency was cancelled, the target operation will be cancelled as well.

For those of you who don't know there's a good updated version of this on github that fixes a bunch of errors: https://github.com/pluralsight/PSOperations

Hey Dave,

Any plans to update the excellent WWDC 2015 sample apps from Swift 2.3, to 3.0?

I have tried doing it myself, but not familiar enough with Swift to get rid of all of the compiler errors.

Or better yet Obj-C, so that we don't need to do this dance every year?