Memory leak from InterceptorRequestChain when ending the chain with returnValueAsync #3057
Comments
|
Hi @marksvend, thanks for the issue and included debugging analysis. It certainly looks like there are paths out of the interceptor request chain that aren't being cleaned up properly. We'll take a look and resolve asap. |
|
I've also been noticing a number of crashes with request chains that started popping up recently as well. Not sure if they are related to this, but here is a stack trace: Note I have not been able to reproduce these crashes. But we have definitely seen an uptick of these crashes since upgrading to 1.2.0.
|
|
Yes, we are also seeing a crash from this code. I was just about to report it. |
|
We are also crashing with our prod app after upgrading to 1.2.0: |
|
I'll be taking a look into this one today. I'm curious about the versioning though since all of the bug reports here indicate "update to |
|
@calvincestari I updated from 1.0.6 to 1.2.0 |
|
We also upgraded from 1.0.6 to 1.2.0 |
|
@marksvend @sgharraph - do either of you use custom interceptors in your request chain? |
|
We do use a few custom interceptors |
|
We also use custom interceptors. You can see that from the stack trace I attached |
|
+1. We are also seeing a crash with a stack trace very similar when updating from 1.0.5 to 1.2.1, and we also use custom interceptors: |
|
This has been a tricky issue to resolve given how we chose to implement HTTP-based subscriptions while maintaining 100% backwards compatibility for existing request chains. I don't think the bugs are related to custom interceptors necessarily but rather how we had to use an I discarded the first solution (#3065) because it required authors of custom interceptors to know too much internal knowledge of how the request chain worked and how it managed the unmanaged instance. I have another solution up at #3070 which removes the unmanaged instance and goes back to the old memory model for the request chain, while still supporting HTTP-based subscriptions, but at the cost of being a minor breaking change and requiring an update to custom interceptors. I'm busy with the last set of tests for that PR and then adding a migration guide which will detail the changes needed. |
|
The fix for this has been merged into the Our docs system doesn't render changes unless the branch is built against Since the solution completely removed the |
|
@calvincestari Great! That seems to have done the trick. Looking forward to official release of 1.3! |
Summary
I'm reporting a memory leak in the iOS Apollo library version 1.2.0 where the request chain does not get deallocated when there is a cache hit.
When a request chain finishes with
returnValueAsyncand does not callproceedAsync, thereleaseManagedSelfnever gets called. The retain cycle never gets broken, and the chain leaks. This typically happens in the success case ofCacheReadInterceptor.returnCacheDataElseFetch.I confirmed that adding a call to
self.releaseManagedSelffromInterceptorRequestChain.returnValueAsyncsolves the memory leak: all of the chain instances get deallocated properly.However, this solution would break the
CacheReadInterceptor.returnCacheDataAndFetchcase, which calls bothreturnValueAsyncandproceedAsync. It wouldn't be right to callreleaseManagedSelfprematurely before the chain has finished. There's no way to know whenreturnValueAsyncis called whether that is the end or not.There's another leak in the
handleErrorAsyncfunction because it does not callself.releaseManagedSelf, either. But that is always the end of the chain so it should be safe to release self there.Version
1.2.0
Steps to reproduce the behavior
I discovered the problem using the memory graph debugger in Xcode after executing many queries in the app. As long as some of them return cached data, a large multi-node memory cycle will appear in the debugger.
Logs
Anything else?
No response
The text was updated successfully, but these errors were encountered: