Thread safety on group cancel

[jcarroll-mediafly]

Prevents internal queue marking the entire groupOperation as finished before cancel flag is set

[jcarroll-mediafly]

Change was small enough I went ahead with it. I started a discussion below where we talk about merits of the proper solution.

#69

[jcarroll-mediafly]

Would like to get other peoples thoughts on this. Is no one else calling cancel on a group operation only to find out in the completion the cancel status is incorrectly false?

[bmkor]

Would like to join the discussion. In your commit, why we need to put the finish inside the cancel look as in line 111 onwards? Is it because it involved state change which is not thread-safe?

[bmkor]

Thanks in advance.

[jcarroll-mediafly]

Correct it all comes down to the state not being thread safe.

In cancel we cancel all the operations, mark the queue suspended as false and finally call super. The actions we need to happen in super may or may not always execute as the first step of canceling operations could cause the group operation to finish() as you pointed out.

Perhaps this lock isn't needed if the order of operations in the cancel change but I've yet to determine a good, reliable, thread-safe fashion to do so.

[bmkor]

Thanks for the reply.

By calling super.cancel(), the group operation may go finish before all its operations reached finish state, right? If yes, wondering if this will cause sth unexpected come out.

[bmkor]

I am just thinking if group op's execute state => its operations' execute state, if not cancelled. Do we need to have its operations' finish state => group op's finish state here.

[jcarroll-mediafly]

Yes, by calling super.cancel() the group operation may finish before all the individual operations finish correct. But before my changes this was always a risk item, either all the ops finished first with the finishingOperation calling finish() or the super.cancel() called finish. Since the self/super.cancel is not thread safe with the observer its a race condition where you could end up with different results. Namely, all the individual ops marked cancelled with the group op reporting just errors (if you pass errors) or all the individual ops marked cancelled with the group op correctly being marked cancelled.

In my opinion, if I cancel a group op that op itself should always be marked cancelled and cascade to its indiviudal operations.

As to how it finishes...I'm open to suggestions.

[bmkor]

Thanks again for the explanation indeed.

Perhaps a bit off topic, when I traced the State change triggered by cancel(), I found that there is a recursive lock in isReady overrided get method.

I feel a bit uncomfortable here. In fact the getter & setter of State was changed to have the same recursive lock which I think was just a lock in the original version of Advanced NSOperation. Mind sharing a bit or I should open a topic for discussion if worthy.

[jcarroll-mediafly]

I did notice the change on the statelock, without knowing the history I'm unsure of the specific reason it was expanded upon. Might want to start a new discussion in my opinion