cancel task before reassigning and don't retain interceptor #1476
Conversation
There was a problem hiding this comment.
To be clear, it's not that I'm philosophically opposed to cancellation, it's that I think it can sometimes obscure other issues when you do it preemptively when it's not necessary. In this case, it shouldn't really be reassigning, because the interceptor shouldn't be getting reused.
| self.currentTask = self.client.sendRequest(urlRequest) { result in | ||
| self.currentTask?.cancel() | ||
|
|
||
| self.currentTask = self.client.sendRequest(urlRequest) { [weak self] result in |
There was a problem hiding this comment.
Good catch on this, I do think this should probably be a [weak self].
| @@ -29,7 +29,13 @@ public class NetworkFetchInterceptor: ApolloInterceptor, Cancellable { | |||
| return | |||
| } | |||
|
|
|||
| self.currentTask = self.client.sendRequest(urlRequest) { result in | |||
| self.currentTask?.cancel() | |||
There was a problem hiding this comment.
Theoretically this should not be necessary since a new NetworkFetchInterceptor is created for each request chain.
There was a problem hiding this comment.
since a new NetworkFetchInterceptor is created for each request chain.
This one is interesting, I haven't seen anywhere that this is requirement. When I was migrating at first I just cached all interceptors but then thought that it might be the case that they "have to be new"
There was a problem hiding this comment.
I added this to the docs on #1484
|
I added the |
What do you think about such change?
From previous interactions I know you might not like canceling bit, but if we don't cancel
currentTaskbefore reassignpublic func cancel()becomes meaningless