Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persisted Query Link: improve memory management #11369

Merged
merged 20 commits into from
Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .api-reports/api-report-link_persisted-queries.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ interface BaseOptions {
// Warning: (ae-forgotten-export) The symbol "ApolloLink" needs to be exported by the entry point index.d.ts
//
// @public (undocumented)
export const createPersistedQueryLink: (options: PersistedQueryLink.Options) => ApolloLink;
export const createPersistedQueryLink: (options: PersistedQueryLink.Options) => ApolloLink & {
resetHashCache: () => void;
};

// @public (undocumented)
interface DefaultContext extends Record<string, any> {
Expand Down
9 changes: 9 additions & 0 deletions .changeset/thick-tips-cry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"@apollo/client": patch
---

Persisted Query Link: improve memory management
* use LRU `WeakCache` instead of `WeakMap` to keep a limited number of hash results
* hash cache is initiated lazily, only when needed
* expose `persistedLink.resetHashCache()` method
* reset hash cache if the upstream server reports it doesn't accept persisted queries
4 changes: 2 additions & 2 deletions .size-limits.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"dist/apollo-client.min.cjs": 38589,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32365
"dist/apollo-client.min.cjs": 38535,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32310
}
63 changes: 62 additions & 1 deletion src/link/persisted-queries/__tests__/persisted-queries.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const giveUpResponse = JSON.stringify({ errors: giveUpErrors });
const giveUpResponseWithCode = JSON.stringify({ errors: giveUpErrorsWithCode });
const multiResponse = JSON.stringify({ errors: multipleErrors });

export function sha256(data: string) {
function sha256(data: string) {
const hash = crypto.createHash("sha256");
hash.update(data);
return hash.digest("hex");
Expand Down Expand Up @@ -151,6 +151,32 @@ describe("happy path", () => {
}, reject);
});

it("clears the cache when calling `resetHashCache`", async () => {
fetchMock.post(
"/graphql",
() => new Promise((resolve) => resolve({ body: response })),
{ repeat: 1 }
);

const hashRefs: WeakRef<String>[] = [];
function hash(query: string) {
const newHash = new String(query);
hashRefs.push(new WeakRef(newHash));
return newHash as string;
}
const persistedLink = createPersistedQuery({ sha256: hash });
await new Promise<void>((complete) =>
execute(persistedLink.concat(createHttpLink()), {
query,
variables,
}).subscribe({ complete })
);

await expect(hashRefs[0]).not.toBeGarbageCollected();
persistedLink.resetHashCache();
await expect(hashRefs[0]).toBeGarbageCollected();
});

itAsync("supports loading the hash from other method", (resolve, reject) => {
fetchMock.post(
"/graphql",
Expand Down Expand Up @@ -517,6 +543,41 @@ describe("failure path", () => {
})
);

it.each([
["error message", giveUpResponse],
["error code", giveUpResponseWithCode],
] as const)(
"clears the cache when receiving NotSupported error (%s)",
async (_description, failingResponse) => {
fetchMock.post(
"/graphql",
() => new Promise((resolve) => resolve({ body: failingResponse })),
{ repeat: 1 }
);
fetchMock.post(
"/graphql",
() => new Promise((resolve) => resolve({ body: response })),
{ repeat: 1 }
);

const hashRefs: WeakRef<String>[] = [];
function hash(query: string) {
const newHash = new String(query);
hashRefs.push(new WeakRef(newHash));
return newHash as string;
}
const persistedLink = createPersistedQuery({ sha256: hash });
await new Promise<void>((complete) =>
execute(persistedLink.concat(createHttpLink()), {
query,
variables,
}).subscribe({ complete })
);

await expect(hashRefs[0]).toBeGarbageCollected();
}
);

itAsync("works with multiple errors", (resolve, reject) => {
fetchMock.post(
"/graphql",
Expand Down
8 changes: 7 additions & 1 deletion src/link/persisted-queries/__tests__/react.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as ReactDOM from "react-dom/server";
import gql from "graphql-tag";
import { print } from "graphql";
import fetchMock from "fetch-mock";
import crypto from "crypto";

import { ApolloProvider } from "../../../react/context";
import { InMemoryCache as Cache } from "../../../cache/inmemory/inMemoryCache";
Expand All @@ -12,7 +13,12 @@ import { createHttpLink } from "../../http/createHttpLink";
import { graphql } from "../../../react/hoc/graphql";
import { getDataFromTree } from "../../../react/ssr/getDataFromTree";
import { createPersistedQueryLink as createPersistedQuery, VERSION } from "..";
import { sha256 } from "./persisted-queries.test";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests failed because we had some kind of concurrency problem - by importing from another test file, all the tests from that other file were added to this file, and then failed for reasons I'll never understand.

That's two hours wasted debugging I'll never get back 🤦


function sha256(data: string) {
const hash = crypto.createHash("sha256");
hash.update(data);
return hash.digest("hex");
}

// Necessary configuration in order to mock multiple requests
// to a single (/graphql) endpoint
Expand Down
Loading