Memory Leak: Mutation + OptimisticResponse makes update function to leak

[kamilkisiela]

After executing a mutation with update function and optimisticResponse the update function is hold in memory forever which leaks other referenced objects.

How to reproduce the issue:
https://github.com/kamilkisiela/apollo-optimistic-response-pessimistic-memory-leak

npm install
npm run start

1. Open chrome://inspector
2. Connect to a process
3. Click play on first breaking point
4. On next breaking point, take a snapshot
5. Look for Foo class

@benjamn I can confirm that Cache.removeOptimistic is called but Foo is still there

apollo-client/src/core/QueryManager.ts

Line 273 in 291e8bd

self.cache.removeOptimistic(mutationId);

[kamilkisiela]

I think that

apollo-client/src/cache/inmemory/readFromStore.ts

Lines 156 to 161 in 2e8cce1

	private executeSelectionSet: OptimisticWrapperFunction<
	[ExecSelectionSetOptions], // Actual arguments tuple type.
	ExecResult, // Actual return type.
	// Arguments type after keyArgs translation.
	[SelectionSetNode, StoreObject | Reference, ReadMergeModifyContext]
	> = wrap(options => this.execSelectionSetImpl(options), {

holds a reference forever because it has no cleanup logic.

I think it's exactly the same issue we had #7086

[benjamn]

@kamilkisiela I've updated the optimism library so that it no longer holds onto entry.args arrays (see benjamn/optimism#104), so memory leaks related to arguments passed to methods like executeSelectionSet should no longer be possible.

Following your reproduction steps, I no longer see retainment paths for Foo objects with lines like args in Entry @1663111 (or anything above it).

These changes were published in optimism@0.13.1 and can be tested by running either npm i optimism@latest or npm i @apollo/client@3.3.0-rc.2.

It might still be worthwhile to evict/forget Entry objects associated with optimistic updates that have been rolled back, but I'd want to see some evidence that's still a problem, now that function arguments are no longer retained.

[kamilkisiela]

Thanks, we're going to test it soon

[smykhailov]

should #7276 be merged into master too? It should minimize amount of potential memory leaks too

[hwillson]

Let us know if this is still a concern with @apollo/client@latest - thanks!