Skip to content

Commit

Permalink
unbreak LiveState unsub when references.size === 0 (#4832)
Browse files Browse the repository at this point in the history
Summary:
fixes #4830 by ensuring that there is only a single path for evicting stuff, ensuring that `maybeResolverSubscription` gets called correctly

Pull Request resolved: #4832

Reviewed By: lynnshaoyu

Differential Revision: D66391741

Pulled By: captbaritone

fbshipit-source-id: a33995d52b4fc61b46bf48513e4a2c5ad306a486
  • Loading branch information
subtleGradient authored and facebook-github-bot committed Dec 13, 2024
1 parent 524a059 commit 1e60e47
Show file tree
Hide file tree
Showing 3 changed files with 208 additions and 26 deletions.
52 changes: 26 additions & 26 deletions packages/relay-runtime/store/RelayModernStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<T: OperationType> = {
query: ConcreteRequest,
variables: VariablesOf<T>,
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 1e60e47

Please sign in to comment.