apollo-client: Memory leak with optimistic response

When using mutations with an optimisticResponse we found a significant increase in memory usage.

Intended outcome: No or only small increase in memory usage.

Actual outcome: Huge increase in memory usage.

How to reproduce the issue: Given an application with multiple watched queries:

• take a memory snapshot
• perform a mutation with an optimistic response (it is sufficient to update a single field of some object)
• take another memory snapshot

Alternatively use the memory profiling function of Chrome instead of taking separate snapshots - this has the advantage that we get callstacks for the allocations.

Versions

• apollo-cache-inmemory: 1.3.8
• apollo-client: 2.4.5
• apollo-link: 1.2.3
• apollo-link-context: 1.0.9
• apollo-link-error: 1.1.1
• apollo-link-http: 1.5.5
• react-apollo: 2.2.4

We suspect that this was introduced in #3394. In particular I suspect the commit 45c4169fa9bf0b4c61e3273254e851e05905a431 to be the main cause.

If InMemoryCache.optimistic contains some entry, read and diff create temporary stores with that optimistic response: https://github.com/apollographql/apollo-client/blob/48a224d504487ea480bce95685402d40c4a90eea/packages/apollo-cache-inmemory/src/inMemoryCache.ts#L137-L139 https://github.com/apollographql/apollo-client/blob/48a224d504487ea480bce95685402d40c4a90eea/packages/apollo-cache-inmemory/src/inMemoryCache.ts#L167-L169 These temporary stores are than used in the calls to readQueryFromStore/ diffQueryAgainstStore and should later get garbage collected. But unfortunately these store objects are internally used as key in calls to cacheKeyRoot.lookup: https://github.com/apollographql/apollo-client/blob/48a224d504487ea480bce95685402d40c4a90eea/packages/apollo-cache-inmemory/src/readFromStore.ts#L124-L130 https://github.com/apollographql/apollo-client/blob/48a224d504487ea480bce95685402d40c4a90eea/packages/apollo-cache-inmemory/src/readFromStore.ts#L144-L150 45c4169fa9bf0b4c61e3273254e851e05905a431 replaced the defaultMakeCacheKey function from optimism (which internally uses a WeakMap) with the newly introduced CacheKeyNode concept, causing these temporary stores to end up as key in one of the (strong) Maps, effectively leaking them.

The more watched queries you have, the bigger the problem because maybeBroadcastWatch calls diff for every single watcher. So we end up creating (and leaking) a temporary store for every single watcher for a single optimistic response (apart from the leakage, there is certainly some room for optimization here).

Not sure if this is the only problem or if there are other leaks as well. We saw a single mutation (that updates a single field) to cause a memory increase of 40-80MBs - not sure if such a huge increase can be caused by this problem alone.

@benjamn Since you were the main author of #3394, could you take a look at this? Let me know if you need more information!