Cancelled Operations never reach the finished state

[t089]

For my own project I also ported the Apple example code from Swift over to Objective-C. I've noticed a fundamental issue however with Apple's implementation of the NSOperation subclass. I wonder, if anyone following this project can confirm my issue:

I think the cancel method is handled incorrectly. The documentation states, that an operation that has been cancelled, should stop its execution and eventually return YES when asked isFinished and NO when asked isExecuting.

In Apple's example code the operation's internal state is set to Cancelled when cancel is invoked. So far so good. However, the method setState: defines Cancelled as a fixed state that cannot be changed. Moreover isFinished will return YES only if state == Finished. This means that a cancelled operation can never report that it is finished.

This behavior can be a serious problem when an operation is added to an operation queue and cancelled during execution. Since the operation will never reach the Finished state, the queue will not remove the operation. In a serial queue this can cause the queue to stall. Subsequent operations will never start.

[kas-kad]

Would it solve the problem? If I change this:

-(BOOL)isFinished
{
    return self.state == KADFinished;
}

to this:

-(BOOL)isFinished
{
    return self.state == KADFinished || self.state == KADCancelled;
}

[t089]

Hi, well, not quite. Let me explain. When an operation is cancelled it does not necessarily mean that it is also finished. In fact, it might still perform some housekeeping before it can actually reach the finished state. isFinished should really only return YES if the operation is in a steady state and cannot perform anything anymore.

In fact, I just redownloaded the Apple Swift example code, which has been silently updated. They removed the Cancelled state completely and instead rely on NSOperation's implementation of the cancel method. This actually makes a lot of sense.

When you call cancel on an NSOperation it sets an internal flag and subsequently returns YES to isCancelled. In your subclass you should check isCancelled regularly during execution. If it turns out to be YES you must stop executing and transition to the Finished state.

So the normal state mutation is always Initialized --> Pending --> EvaluatingConditions --> Ready --> Executing --> Finishing --> Finished

If you cancel the operation it will either skip the Executing state or abort the Executing state and move to Finishing and eventually Finished. The cancellation state therefore must be stored in a separate variable because it is superimposed on the internal state.

There are actually some more changes in the Swift code. I haven't yet looked at them all.

[kas-kad]

Didn't know about the sample code updates, very interesting one!

[Goles]

Interested to know what other changes did you find :)

[kas-kad]

Just ported some changes from apple sample code

[t089]

Good job!

I wonder why they now call super in the start method of the NSOperation subclass. In the docs it clearly says:

In a concurrent operation, your start method is responsible for starting the operation in an asynchronous manner. Whether you spawn a thread or call an asynchronous function, you do it from this method. [...]

[...] At no time in your start method should you ever call super. When you define a concurrent operation, you take it upon yourself to provide the same behavior that the default start method provides, which includes starting the task and generating the appropriate KVO notifications. [...]

Maybe something has changed or it is a mistake...

[kas-kad]

as written in comments:

// NSOperation.start() contains important logic that shouldn't be bypassed.

so contradictory

[Goles]

Actually, I think it might be a mistake.

It's a pretty bad idea to call [super start] inside an NSOperation subclass

[kas-kad]

Would it be a concious decision to call super start? Yes, because we see the comments about it. For me its more likely there is a mistake in documentations, but who knows

[Goles]

Have you actually verified that main() get's called at all?

Sent from my iPhone

On Sep 11, 2015, at 12:51 AM, Andrey Kadochnikov notifications@github.com wrote:

A mistake with comments above? More likely there is a mistake in documentations.

— Reply to this email directly or view it on GitHub.

[t089]

Can you ping someone on github? @davedelong might be able to help...

[kas-kad]

@Goles https://monosnap.com/file/U3ufnKyTCGVxHfIqfb7OFz5v4TBRgp.png

[kas-kad]

I think there is a docs mistake since that time when start had been called from the thread from which the operation was enqueued. They just forgot to remove some outdated parts from the docs. These days the start is always called from separate thread, plus, as soon as we always enqueue our operations there is no need to make them concurrent/asynchronous, therefore there is nothing scary to call super.start.

[Goles]

I'll further dig on this topic... Seems strange.

Also, since all the ops are non-concurrent and use the specialized queue subclass... Why are they overriding start in the first place?

[warpfactorfive]

If you don't override start then you can't explicitly maintain state in your own code. You'll typically see people use _isExecuting and _isFinished variables to provide KVO compliance, because the base NSOperation class hides it's own implementation. You can't easily extend it's behavior through subclassing so you wind up having to override start, finish, isExecuting and isFinished to reference your own properties.

[warpfactorfive]

PS -- I'm still making my way through this stuff myself. I loved the WWDC session, but hate the fact that they released the code in Swift only.

[warpfactorfive]

Regarding calling [super start], found this in the Apple docs:

https://developer.apple.com/library/mac/documentation/Cocoa/Reference/NSOperation_class/

IMPORTANT

At no time in your start method should you ever call super. When you define a concurrent operation, you take it upon yourself to provide the same behavior that the default start method provides, which includes starting the task and generating the appropriate KVO notifications.

Why Apple code violates their own prohibition is beyond me.

[kas-kad]

If someone missed this one. Dave DeLong:

If you’re interested in staying up-to-date with the Advanced NSOperations sample code, I’d recommend this repo: https://github.com/pluralsight/PSOperations