cancel should not call finish, should it?

[t089]

I am a bit surprised to see that the cancel implementation of Operation calls finish by itself, if state > .Ready. Shouldn't cancel only mark the operation as cancelled ? Isn't it the responsibility of the operation to check for this flag, and if true, stop its work, clean up and only then transition to the finished state? Or am I missing something?

[jcarroll-mediafly]

Yeah, I struggled with this a bit myself. Especially when using group operations and dealing with race conditions.

If you haven't seen my reported issue I would like to hear other's thoughts on it. Shameless plug for advice :D

#69

[MarkQSchultz]

@t089 I agree. This was pointed out quite a while ago too. One of the reasons we haven't changed it yet is that it's a pretty big breaking change.

[t089]

Ok, well I guess it is a big change, but it is also quite a big mistake in the implementation. In its current form a call to cancel will immediately fire the completion observers and clients would think that the op finished its work, while in reality it might still be very busy and do stuff. Even worse, when it is done it can't call the didFinish observer again because it already fired.

[MarkQSchultz]

You do realize that I already said that I agree with you, right? Not sure the point of most of that comment...

[t089]

Yup just wanted to add some more arguments pro change. Sorry if you feel it was redundant.

You do realize that I already said that I agree with you, right? Not sure the point of most of that comment...

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[MarkQSchultz]

👍This would be a good change. Just a big one at this point. That's all. I would want to version the framework on this change though, as it is a breaking change.

[priteshshah1983]

It doesn't look like this change was ever made (or merged). Can you please confirm?