apollo-ios

apollo-ios copied to clipboard

Feature request

When a request chain gets cancelled, it calls the interceptors and cancels them if they adhere to Cancellable. However, it's not clear why that isn't also done with the error interceptor.

Motivation

This would allow a client using the SDK to write a single error interceptor and get notified if a request chain is cancelled without having to write a separate interceptor just to be notified of cancellation.

Clients that don't use an error interceptor or don't make that interceptor adhere to Cancellable would be unaffected.

Proposed solution

Add code that checks if there is an additional error interceptor, and if it adheres to Cancellable, it would cancel that one as well.

Outstanding Questions

@PatrickDanino So basically adding the following to cancel:

    if
      let additionalHandler = self.additionalErrorHandler,
      let additionalCancelable = additionalHandler as? Cancellable {
        additionalCancelable.cancel()
    }

is what you're looking for, correct?

Correct, assuming at the end of RequestChain.cancel().

For full transparency, after trying to implement this, I ended up having to use an interceptor anyway.

The reason is that I needed to keep track of data in the request, and unfortunately the cancel() call doesn't provide any context for the ongoing request. We need this to be able to clean up some request-specific data.

🤔 In general the requests are supposed to be pretty lightweight - what kind of data are you needing to clean up?

The short answer is that we track each request we make and need to know if it gets cancelled. In our particular case, the code that can cancel a request is not the same as the one that needs to know the request was cancelled.

To use a simple example that relates back to the interceptor samples on the Apollo website, imagine simply wanting to log that a particular request was cancelled. The code that initiates the request shouldn't need to know or have to do any work for that logging to happen - it only needs to worry about calling cancel() when appropriate. In turn, a lower layer responsible for logging may want to be notified and display information about the request.

As far as I can tell, there's no way to do that with interceptors and the current Cancellable protocol since it doesn't pass in the current state of the request/response.

Rather than only check for Cancellable conformance, one option would be to add a func cancel<Operation: GraphQLOperation>(request: HTTPRequest<Operation>, response: HTTPResponse<Operation>?) to the ApolloInterceptor interface (and/or ApolloErrorInterceptor interface), as well as add an extension that does nothing by default to avoid any backwards compatibility issues.

So we've had a separate request to start throwing cancellation errors when a request is cancelled (basically following the NSURLSession pattern). If we did that, then every time something was called you'd get an error you could check with an additionalErrorInterceptor for the cancellation error.

Would that work for you? I think that would probably be simpler, but I'd like to make sure that would work with your use case.

That should work for us. In that case, I presume we would just need to add a check for this error in the additionalErrorInterceptor instance and make sure to call defer { completion(.failure(error)) } from within handleErrorAsync(...).

And just to confirm, though technically a tangential issue:

If we did not have a custom error interceptor, would the result handler for the request that is cancelled get called by default with the cancellation error? That is in no small part what prompted our original request as we realized that we couldn't always rely on the result callbacks to always get called.

While we would prefer that the result handler always get called, we also understand why it was originally designed the way it is as it could require every client to properly handle the 'cancel' error case.

Yeah, definitely originally designed not to force every client to handle cancellation errors but after a lot of talking with folks now I see why Apple went that way with URLSession 🙃

I was also surprised to find the result handler is not called on cancellation. Always having the completion handler called is a requirement if clients want to wrap operations to support async/await (or if this library wanted to support it!)

Imo this would be a significant breaking change, since clients may be making assumptions around the current behavior. So, it seems important to have this in the 1.0 release (if it's not already)