apollo-ios
apollo-ios copied to clipboard
apollographql
[NetworkFetchInterceptor] Don't early exit during request completion if the task was cancelled
I'm wondering why this guard stmt exists? When a task is cancelled, the result object will contain an error Domain=NSURLErrorDomain Code=-999 "cancelled". Shouldn't we let the completion handlers run? This was causing a request to appear as if it was hanging because the completion was never called.
@pm-dev: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/
@pm-dev: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/
Done
So this is actually checking whether the request chain itself has had cancel called on it, rather than checking the URLSessionTask directly. This is something that can only be done by a developer, on purpose.
The expectation is that if the developer actively called cancel on the task, they don't want to proceed with further parsing since they don't actually need the result.
@pm-dev Does that help explain why this is the way it is? What is the use case you're trying to fix here?
@pm-dev Does that help explain why this is the way it is? What is the use case you're trying to fix here?
@designatednerd Thanks for the quick reply and following up!
My use case is that I put Apollo requests into a queue and it's possible for that queue to become suspended then have all its operations cancelled. At the same time, my code was making the assumption that a fetch or mutation result handler is guaranteed to be called.
There might not be a right/wrong answer here. On one hand, as you said, the developer actively called cancel and could in theory, manually trigger whatever needed to happen in the result handler. On the other hand, making the guarantee that the result handler is called makes life a lot simpler for developers.
For example, Apple decided to go with the approach of still notifying the callback when a URLSessionTask is cancelled -NSURLSessionTaskDelegate still gets notified via urlSession(_: task: didCompleteWithError:). Think about it they didn't in Apollo's case; you'd have to update Apollo.URLSessionClient's tasks from a second place -right after calling cancel on the task.
At the same time, my code was making the assumption that a fetch or mutation result handler is guaranteed to be called.
I agree with this assumption. Even if a task is canceled by the developer, the result handler should be called and tell the call site that the request was canceled.
I'm not sure if that's all that this PR does and it doesn't have any other side effects, so I'm not prepared to accept the PR yet, but I do think that expected behavior should be to notify the call site of the cancellation.
Any news on this @AnthonyMDev ? I have noticed the same problem, @pm-dev did you manage to find a solution for your use case ?
@petarbelokonski we haven't advanced this PR anymore from @AnthonyMDev's comment. Given that it was a while ago and there are now conflicts to resolve I'm going to close this PR and if the author wants to take it up again after syncing with main we can do that. The current state is still that the closure will not be called on cancellation.
tl;dr
While i agree that the existing behavior is not the best choice, changing it will break things for existing users. But regardless, this PR is not complete and does not solve the problem. I don't expect our team to invest time in working on this until we start working on the 2.0 release. If someone from the community wants to put together a new PR that solves this problem in its entirety in a way that does not break code that relies on the existing behavior, we would be happy to collaborate and get something merged in!
Re-reading this thread, I realize I was not clear here. My apologies.
Breaking Existing Behavior
While I do agree that we should be calling the resultHandler on a cancellation, I am concerned about this be a notable change in behavior that will likely break a lot of people's existing code. I could see a lot of situations where this change would lead to apps that start displaying error states when they shouldn't (or other issues).
People have a lot of code that currently works on the assumptions of existing behavior, so changing that behavior can be a problem. I'm okay with making minor breaking changes that actually cause people's code to fail to compile unless they fix it. But a hidden behavioral change in a minor version that isn't a 100% indisputable bug fix can have severe consequences.
It's okay to do something like this on a major version bump. This leads me to believe this change should be made for the 2.0 release. That won't be coming for a while, and it's going to involve replacing the interceptor chain entirely. To ensure that we address this behavior in the 2..0 version, I've linked this PR to the 2.0 milestone so we can reference it then.
If we really wanted to do this in a minor version, I think it should be accompanied by a deprecation of some existing API and the behavior should be changed only in the new API so that the behavior only changes once you update away from the deprecated code. I'm not really sure how we would do that without duplicating a LOT of code and making maintenance painful.
Alternatively, we could add this as an option on RequestChainNetworkTransport that would allow users to opt-in to the new behavior. That might be a better approach.
Incomplete Implementation
However, this particular PR was not a complete solution. If we were to make this change, it would need to be more robust than this.
The isCancelled property is checked in multiple places, and this only addresses one of them. And if the cancellation happens due to something that isn't a networking task being cancelled, removing this line of code would currently result in other interceptors failing due to not having the data they needed. It wouldn't be clear from the returned error that the reason for the "error" was because of a cancellation.
Ideally, we would actually have the InterceptorRequestChain be checking for isCancelled after each individual interceptor proceeds, and then calling completion with a custom ChainError.requestCancelled.
The interceptors that exist today would not need to check for isCancelled if the request chain itself was doing that, so we could then remove those checks.
It is theoretically possible that an interceptor that was doing heavy, multi-step async operations would still want to be able to check for isCancelled and early exit, so it should be documented that interceptors are always expected to call proceedAsync, even on cancellation. Though I would also argue that any interceptor that was doing multiple async steps should probably be refactored into 2 separate interceptors for this very reason.