Cause a failure when an operation is cancelled #693

Merged
merged 1 commit into from Dec 26, 2012

Projects

None yet

7 participants

@danielctull
Contributor

At present, when an operation is cancelled there is no decent way to handle that within the scope of AFNetworking. This change uses the existing error created when a cancel is called and uses that as the error for the operation. This causes the failure block to be executed.

Fixes issue #657.

@mattt
Contributor
mattt commented Dec 26, 2012

After giving it a lot of thought, I have to agree with your assessment of all of this. Yes—canceling and operation should trigger the failure callback. Indeed, canceling the operation cancels the NSURLConnection, which finishes with an error. Doing anything else would be inconsistent with the contract of AFHTTPClient. It is also far easier to check against a cancelled operation in a failure block to maintain the same behavior than to have to reimplement setCompletionBlockWithSuccess:failure:.

Because this is a potentially breaking change, I'll be tagging this as part of a new release of AFNetworking later today.

Thanks again for putting up with my stubbornness about this—I'm not exactly used to being conservative about much in my everyday life, so reconciling that with my code stewardship habits is something I'm still trying to figure out :p

@mattt mattt merged commit 7134e96 into AFNetworking:master Dec 26, 2012
@PeteC
PeteC commented Jan 3, 2013

Thanks for putting up with mine and @danielctull's stubbornness too!

@iwarshak

Just want to chime in disagreeing with this. I think merging the concept of failure & cancellation leads to extra code for the developer.

For instance, if a UIViewController uses AFN to load some data, but that view is poppe & dealloced before the success/failure blocks can be handled. Currently it seems the best solution is to keep an set of current operations, and #cancel them on dealloc. With this change, all failure blocks now have to check and see if this was truly a failure, or if this 'failed' because it was explicitly cancelled. (So there has to be an if/else branch in the failure block, which seems prone to ... 😎 ....failure. YEAHHHHHH!!!)

If it was cancelled, it's done explicitly, and the dev can explicitly handle that case right where the operation was cancelled.

Couldn't this be handled by posting a notification instead? Interested parties can just listen for that notification. (AFOperationCancelledNotification)

@blakewatters
Contributor

@iwarshak you could also nil out the completion blocks when doing your cancel in dealloc

@mattt
Contributor
mattt commented Jun 22, 2013

@iwarshak At most, it's 2 extra lines to return early if [operation isCancelled] == YES. It's a much greater inconvenience the other way around. AFNetworking has a clear contract, that if the error property of a request operation is non-nil, then the operation should trigger failure. Canceling an operation triggers connection:didFailWithError:, and that error gets stored in the operation. Making an exception here would be inconsistent for no particular reason.

@0xced
Collaborator
0xced commented Jun 22, 2013

Canceling an operation triggers connection:didFailWithError:

No, canceling a NSURLConnection just cancels the connection without producing any error. As per -[NSURLConnection cancel] documentation:

After this method is called, the connection’s delegate no longer receives any messages for the connection.

Actually, the error is explicitly constructed and assigned in -[AFURLConnectionOperation cancelConnection]

I’d like to join @iwarshak and ask you to reconsider this decision. I think it doesn’t make sense to get an error with NSURLErrorCancelled when the programmer explicitly wrote [connectionOperation cancel]. This perfectly aligns with the rule of delegate methods not being called when the programmer explicitly calls the corresponding method. E.g. calling selectRowAtIndexPath:animated:scrollPosition: does not cause the delegate to receive a tableView:willSelectRowAtIndexPath: or tableView:didSelectRowAtIndexPath: message.

@0xced
Collaborator
0xced commented Jun 28, 2013

Maybe not a good idea in the 1.x branch but I'd love to see that change reversed in the 2.0 branch.

@nfarina
nfarina commented Feb 24, 2014

This bit me today - took me a while to realize why my connections were "failing" after I was properly canceling them. It feels like Apple's classes (such as NSURLConnection as pointed out by @0xced) tend to adhere to the contract of "If you call cancel, I will never bother you again and you can safely forget about me" which this behavior violates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment