Crash in 0.36.0

[RolandasRazma]

Bug report

Crash using interceptors

Versions

Please fill in the versions you're currently using:

• apollo-ios SDK version: 0.36.0
• Swift version: 5.3

Steps to reproduce

I have TokenAddingInterceptor that retrieves token from server and asynchronously calls chain.proceedAsync. I have check for TokenAddingInterceptor being released to not call up the chain on dealloc, so that's not the case.

[designatednerd]

The crash is happening in the URLSessionClient rather than in any of the interceptors - it looks like the client either a) Hasn't had a chance to set up its URLSession or b) has had the session torn down.

Can you share how you're setting up your TokenAddingService?

[RolandasRazma]

There is nothing interesting to see there. It just check for token validity if it it expired updates it from server and calls back. I will try to trace/fix crash as it's blocking us, but as its another race condition its not going to be simple as I can't even replicate it reliably. Sometimes it crashes every run (tests) sometimes I hit #1376 and sometimes all tests passes without any problem :)

[RolandasRazma]

I can replicate it with example from https://www.apollographql.com/docs/ios/tutorial/tutorial-mutations/

class TokenAddingInterceptor: ApolloInterceptor {
    func interceptAsync<Operation: GraphQLOperation>(chain: RequestChain, request: HTTPRequest<Operation>, response: HTTPResponse<Operation>?, completion: @escaping (Result<GraphQLResult<Operation.Data>, Error>) -> Void) {
        DispatchQueue.main.async { [weak self] in
            guard self != nil else { return }
            chain.proceedAsync(request: request, response: response, completion: completion)
        }
    }
    
}

[RolandasRazma]

This is how memory graph looks for crashing client

[RolandasRazma]

NetworkFetchInterceptor:32 don't you need self.currentTask?.cancel() before it?

P.S. adding cancel didn't stop crash, but still, don't you need it there? (made PR #1476)

[RolandasRazma]

Ok, what's happening is that RequestChain (created in RequestChainNetworkTransport) lifetime is longer than LegacyInterceptorProvider and LegacyInterceptorProvider does shouldInvalidateClientOnDeinit

RequestChain is created in RequestChainNetworkTransport, LegacyInterceptorProvider is released invalidating client and RequestChain knows nothing about it.

I managed to "solve" it by retaining link to chain in RequestChainNetworkTransport and canceling it in deinit - not sure if that's correct thing to do, as that would imply that I need to cancel it on send as well (or retain all references created by send)

[RolandasRazma]

sorry for spam @designatednerd

[RolandasRazma]

I thought not retaining chain would help

class TokenAddingInterceptor: ApolloInterceptor {
    func interceptAsync<Operation: GraphQLOperation>(chain: RequestChain, request: HTTPRequest<Operation>, response: HTTPResponse<Operation>?, completion: @escaping (Result<GraphQLResult<Operation.Data>, Error>) -> Void) {
        DispatchQueue.main.async { [weak self, weak chain] in
            guard self != nil else { return }
            chain?.proceedAsync(request: request, response: response, completion: completion)
        }
    }
    
}

but that's not the case - it is deallocated before we can update token and call to server never executed

[designatednerd]

right - if you let go of the chain there there's nothing hanging on to it, so ARC smashes it.

I'm surprised the legacy interceptor provider is getting deallocated - that indicates that the Request Chain Network Transport itself is getting deallocated. I would really like to see how you're setting this stuff up for tests - it seems like maybe something is calling into an old instance of RCNT in different tests.

[RolandasRazma]

RequestChainNetworkTransport is deallocated, but the problem is not that it is deallocated, the problem is that it doesn't cleanup properly while it does that

class TokenAddingInterceptor: ApolloInterceptor {
    func interceptAsync<Operation: GraphQLOperation>(chain: RequestChain, request: HTTPRequest<Operation>, response: HTTPResponse<Operation>?, completion: @escaping (Result<GraphQLResult<Operation.Data>, Error>) -> Void) {
        DispatchQueue.main.asyncAfter(deadline: .now() + .seconds(1)) { [weak self] in
            guard self != nil else { return }
            chain?.proceedAsync(request: request, response: response, completion: completion)
        }
    }
}

let transport = RequestChainNetworkTransport(...)
let apolloClient = ApolloClient(networkTransport: transport, ...)
apolloClient.fetch(query: ...)

will make whole app crash in 1 second because ApolloClient will be deallocated together with RequestChainNetworkTransport while RequestChain will be retained by block (and it has to be)

Crash will happen because deallocatingin LegacyInterceptorProvider will call client.invalidate() that will nil URLSessionClient.sesion and after 1 sec it will be accessed with force unwrap

[RolandasRazma]

this is the crash #1480

[RolandasRazma]

I guess easy workaround would be to remove force unwrap, but the real problem is that RequestChain don't know that it is no longer needed. Correct fix would be to add "cancel" for chain in RequestChainNetworkTransport as mentioned in #1473 (comment) the only difficulty it that there is multiple chains per transport and all of them needs to be cancelled

[RolandasRazma]

Created PR with workaround #1481 but somehow don't fee good about it :) It doe's work, but feels like some code smell

[RolandasRazma]

should I close this?

[designatednerd]

I'll close it when it actually ships

[designatednerd]

0.37.0 has shipped!