diff --git a/packages/relay-runtime/store/RelayModernStore.js b/packages/relay-runtime/store/RelayModernStore.js index 11b6ec78490d..4db26b083d69 100644 --- a/packages/relay-runtime/store/RelayModernStore.js +++ b/packages/relay-runtime/store/RelayModernStore.js @@ -764,36 +764,36 @@ class RelayModernStore implements Store { } } - // Sweep records without references - if (references.size === 0) { - // Short-circuit if *nothing* is referenced - this._recordSource.clear(); - } else { - // Evict any unreferenced nodes - const storeIDs = this._recordSource.getRecordIDs(); - for (let ii = 0; ii < storeIDs.length; ii++) { - const dataID = storeIDs[ii]; - if (!references.has(dataID)) { - const record = this._recordSource.get(dataID); - if (record != null) { - const maybeResolverSubscription = RelayModernRecord.getValue( - record, - RELAY_RESOLVER_LIVE_STATE_SUBSCRIPTION_KEY, - ); - if (maybeResolverSubscription != null) { - // $FlowFixMe - this value if it is not null, it is a function - maybeResolverSubscription(); - } - } - this._recordSource.remove(dataID); - if (this._shouldRetainWithinTTL_EXPERIMENTAL) { - // Note: A record that was never retained will not be in the roots map - // but the following line should not throw - this._roots.delete(dataID); + // NOTE: It may be tempting to use `this._recordSource.clear()` + // when no references are found, but that would prevent calling + // maybeResolverSubscription() on any records that have an active + // resolver subscription. This would result in a memory leak. + + // Evict any unreferenced nodes + const storeIDs = this._recordSource.getRecordIDs(); + for (let ii = 0; ii < storeIDs.length; ii++) { + const dataID = storeIDs[ii]; + if (!references.has(dataID)) { + const record = this._recordSource.get(dataID); + if (record != null) { + const maybeResolverSubscription = RelayModernRecord.getValue( + record, + RELAY_RESOLVER_LIVE_STATE_SUBSCRIPTION_KEY, + ); + if (maybeResolverSubscription != null) { + // $FlowFixMe - this value if it is not null, it is a function + maybeResolverSubscription(); } } + this._recordSource.remove(dataID); + if (this._shouldRetainWithinTTL_EXPERIMENTAL) { + // Note: A record that was never retained will not be in the roots map + // but the following line should not throw + this._roots.delete(dataID); + } } } + if (log != null) { log({ name: 'store.gc.end', diff --git a/packages/relay-runtime/store/__tests__/resolvers/ResolverGC-test.js b/packages/relay-runtime/store/__tests__/resolvers/ResolverGC-test.js index 661d4be55daa..d6cf747fc063 100644 --- a/packages/relay-runtime/store/__tests__/resolvers/ResolverGC-test.js +++ b/packages/relay-runtime/store/__tests__/resolvers/ResolverGC-test.js @@ -827,6 +827,85 @@ test('Resolver reading a client-edge to a client type (recursive)', async () => }); }); +test.each([0, 1, 5])( + 'Live Resolver cleanup when %i references retained', + async numRetainedReferences => { + const unsubscribeMock = jest.fn(); + const subscribeSpy = jest + .spyOn(GLOBAL_STORE, 'subscribe') + .mockImplementation(() => { + return unsubscribeMock; + }); + + // Reset the store before each test run + resetStore(); + + const source = RelayRecordSource.create(); + + const store = new RelayModernStore(source, { + gcReleaseBufferSize: 0, + }); + + const environment = new RelayModernEnvironment({ + network: RelayNetwork.create((request, variables) => { + return Promise.resolve({data: {}}); + }), + store, + }); + + // The operation that uses the live resolver + const operation = createOperationDescriptor( + graphql` + query ResolverGCTestNoRetainedQueriesQuery { + counter_no_fragment + } + `, + {}, + ); + + // Execute the query to populate the store + await environment.execute({operation}).toPromise(); + + // Lookup the data to trigger evaluation of the resolver + const snapshot = environment.lookup(operation.fragment); + + // Ensure the live resolver has been called + expect(subscribeSpy).toHaveBeenCalledTimes(1); + expect(snapshot.data).toEqual({counter_no_fragment: 0}); + + // Retain the operation if numRetainedReferences > 0 + const retains = []; + for (let i = 0; i < numRetainedReferences; i++) { + retains.push(environment.retain(operation)); + } + + // Run GC + store.__gc(); + + if (numRetainedReferences > 0) { + // The data is still retained, so cleanup should not have happened + expect(unsubscribeMock).not.toHaveBeenCalled(); + } else { + // The data is not retained, cleanup should have happened + expect(unsubscribeMock).toHaveBeenCalledTimes(1); + } + + // Dispose of the retains + for (const retain of retains) { + retain.dispose(); + } + + // Run GC again to ensure cleanup happens after disposing retains + store.__gc(); + + // Now, cleanup should have happened if it didn't before + expect(unsubscribeMock).toHaveBeenCalledTimes(1); + + // Cleanup the spy + subscribeSpy.mockRestore(); + }, +); + type TestProps = { query: ConcreteRequest, variables: VariablesOf, diff --git a/packages/relay-runtime/store/__tests__/resolvers/__generated__/ResolverGCTestNoRetainedQueriesQuery.graphql.js b/packages/relay-runtime/store/__tests__/resolvers/__generated__/ResolverGCTestNoRetainedQueriesQuery.graphql.js new file mode 100644 index 000000000000..18432b4d6ee1 --- /dev/null +++ b/packages/relay-runtime/store/__tests__/resolvers/__generated__/ResolverGCTestNoRetainedQueriesQuery.graphql.js @@ -0,0 +1,103 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @oncall relay + * + * @generated SignedSource<<5f279865ba329845f19bc4bed597d3b5>> + * @flow + * @lightSyntaxTransform + * @nogrep + */ + +/* eslint-disable */ + +'use strict'; + +/*:: +import type { ClientRequest, ClientQuery } from 'relay-runtime'; +import type { LiveState } from "relay-runtime"; +import {counter_no_fragment as queryCounterNoFragmentResolverType} from "../LiveCounterNoFragment.js"; +import type { TestResolverContextType } from "../../../../mutations/__tests__/TestResolverContextType"; +// Type assertion validating that `queryCounterNoFragmentResolverType` resolver is correctly implemented. +// A type error here indicates that the type signature of the resolver module is incorrect. +(queryCounterNoFragmentResolverType: ( + args: void, + context: TestResolverContextType, +) => LiveState); +export type ResolverGCTestNoRetainedQueriesQuery$variables = {||}; +export type ResolverGCTestNoRetainedQueriesQuery$data = {| + +counter_no_fragment: ?number, +|}; +export type ResolverGCTestNoRetainedQueriesQuery = {| + response: ResolverGCTestNoRetainedQueriesQuery$data, + variables: ResolverGCTestNoRetainedQueriesQuery$variables, +|}; +*/ + +var node/*: ClientRequest*/ = { + "fragment": { + "argumentDefinitions": [], + "kind": "Fragment", + "metadata": null, + "name": "ResolverGCTestNoRetainedQueriesQuery", + "selections": [ + { + "kind": "ClientExtension", + "selections": [ + { + "alias": null, + "args": null, + "fragment": null, + "kind": "RelayLiveResolver", + "name": "counter_no_fragment", + "resolverModule": require('./../LiveCounterNoFragment').counter_no_fragment, + "path": "counter_no_fragment" + } + ] + } + ], + "type": "Query", + "abstractKey": null + }, + "kind": "Request", + "operation": { + "argumentDefinitions": [], + "kind": "Operation", + "name": "ResolverGCTestNoRetainedQueriesQuery", + "selections": [ + { + "kind": "ClientExtension", + "selections": [ + { + "name": "counter_no_fragment", + "args": null, + "fragment": null, + "kind": "RelayResolver", + "storageKey": null, + "isOutputType": true + } + ] + } + ] + }, + "params": { + "cacheID": "a5905620d7505500bf0cb987c540bc24", + "id": null, + "metadata": {}, + "name": "ResolverGCTestNoRetainedQueriesQuery", + "operationKind": "query", + "text": null + } +}; + +if (__DEV__) { + (node/*: any*/).hash = "c081f9d7220711c53528ae28f136ca80"; +} + +module.exports = ((node/*: any*/)/*: ClientQuery< + ResolverGCTestNoRetainedQueriesQuery$variables, + ResolverGCTestNoRetainedQueriesQuery$data, +>*/);