Completing NSOperation before executing callback

[bravedrake]

I've just updated an old project from AFNetworking 1.x to 3.1, and found your wrapper very useful.

Maybe this change seems a little pedantic, but it solves an issue I had where I inspect the number of operations in the queue in the success callback. With AFNetworking 1.x code, the operation would have been removed from the queue before the callback, meaning I could see if the queue was empty. This change fixes the issue.

[pronebird]

I wonder what's the standard behavior of NSOperation.completionHandler? Probably would make sense to match up somehow for the sake of consistency.

[robertmryan]

The standard NSOperation completion block is called after the operation finishes. What this means is that other operations that are dependent upon the current operation will generally have started before the completion handler is called. Having said that, there are no assurances as to which thread this default completion handler could be called, so I believe that you technically have a race condition. So, I would hesitate to ever put code in my completion handler that was dependent upon the state of what operations were remaining on the queue. Personally, I would use operation dependencies rather than looking at the count of operations remaining on the queue. That is guaranteed to work.

In terms of why I implemented it like I did, I didn't change the behavior consciously, but rather just because this is how I've always done it and because I personally hate this "call completion block after dependent operations probably have started" behavior. It's equivalent to a notification (that you know it will fire some time after the operation finishes, but no assurances of precisely when). For example, let's say you queue an authentication operation and a bunch of requests that are dependent upon that authentication finishing. But, if the authentication request failed, I'd like to cancel the other network operations that were queued and dependent upon the authentication operation. But if the completion callback takes place after the authentication operation completes, I don't have an opportunity to cancel those other operations. You could achieve this by employing some isReady logic, but that's more complicated. Also, if you went crazy and wrote some rich mechanism for passing data between dependent operations, this is more easily achieved if the block fires before the dependent operations start.

So while I wrote this for (what I consider to be) well considered reasons, in retrospect I think that you're right, that this isn't consistent with how AFNetworking has historically handled its completion blocks. I should probably reconsider this. If you don't mind, I'm going to stew on this a bit (and give others a chance to chime in) before I change this.

[bravedrake]

Thanks for the quick response to this.

I agree that it would be better if the completion block of an NSOperation was called before a subsequent operation was executed.

You are right that logic in the callbacks should not be relying on state of other operations in the queue. I am working with legacy code that I am trying to bring up to date, and having looked at my particular case again, I think the best approach for me is actually to not use an NSOperationQueue at all, so I won't actually be using AFHTTPSessionOperation anymore.

I'll leave it up to you to mull it over anyhow.

[robertmryan]

Re your retiring of NSOperation, that's certainly fine. Personally, I've got no problems with NSOperation based solution (I think it's a really elegant way to manage dependencies, degree of concurrency, identifying when all operations are done, etc.), and I think it's a little sad that AFNetworking retired it. I suspect he retired it, though, because the whole "operation" paradigm falls apart when you start to consider background requests.

Re this particular push request, as I review it, I see you've changed the sequence in which operations are completed and when the completion handler is called. But if I started to pull on this thread (make it consistent with AFHTTPRequestOperation), a more comprehensive refactoring might be called for. Namely the ability to specify the completion queue, support for dispatch groups, etc.

My final reservation is that this would be a non-backward compatible change to this repo (not that there are enough people using it, nor are the likely to be doing anything contingent on the old behavior). But if I'm going to make a non-backward compatible change, I'd probably be inclined to do this within the context of the more radical refactoring.

Bottom line, all said and done, unless there is an outpouring of feedback voting for this pull request, or more dramatic refactoring, I'm inclined to let sleeping dogs lie. Let's see what feedback we get on this pull request and make a decision from there.