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.