From e241b808c1b6b1aa82d72c1438e1764a1fad9ddd Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 27 Feb 2023 22:32:10 -0500 Subject: [PATCH 1/4] Add WeakRef and FinalizationRegistry to createCache --- .../src/routes/examples/streaming-cache.tsx | 1 - packages/suspense/src/cache/createCache.ts | 70 +++++++++++++++---- .../src/cache/createSingleEntryCache.ts | 16 ++--- .../suspense/src/utils/WeakRefMap.test.ts | 39 +++++++++++ packages/suspense/src/utils/WeakRefMap.ts | 52 ++++++++++++++ packages/suspense/src/utils/stringify.ts | 20 ++++++ tsconfig.json | 2 +- 7 files changed, 176 insertions(+), 24 deletions(-) create mode 100644 packages/suspense/src/utils/WeakRefMap.test.ts create mode 100644 packages/suspense/src/utils/WeakRefMap.ts create mode 100644 packages/suspense/src/utils/stringify.ts diff --git a/packages/suspense-website/src/routes/examples/streaming-cache.tsx b/packages/suspense-website/src/routes/examples/streaming-cache.tsx index 1652f2a..004711d 100644 --- a/packages/suspense-website/src/routes/examples/streaming-cache.tsx +++ b/packages/suspense-website/src/routes/examples/streaming-cache.tsx @@ -22,7 +22,6 @@ export default function Route() { to render a larger data set as it incrementally loads.

Click the "start demo" button to fetch user data in the cache.

- {/**/} diff --git a/packages/suspense/src/cache/createCache.ts b/packages/suspense/src/cache/createCache.ts index 0a7a732..911e172 100644 --- a/packages/suspense/src/cache/createCache.ts +++ b/packages/suspense/src/cache/createCache.ts @@ -17,16 +17,27 @@ import { import { assertPendingRecord } from "../utils/assertPendingRecord"; import { isThenable } from "../utils/isThenable"; import { isPendingRecord } from "../utils/isPendingRecord"; +import { WeakRefMap } from "../utils/WeakRefMap"; + +export type CreateCacheOptions, Value> = { + config?: { + useWeakRef?: boolean; + }; + debugLabel?: string; + getKey?: (...params: Params) => string; + load: (...params: [...Params, CacheLoadOptions]) => Thenable | Value; +}; // Enable to help with debugging in dev const DEBUG_LOG_IN_DEV = false; -export function createCache, Value>(options: { - load: (...params: [...Params, CacheLoadOptions]) => Thenable | Value; - getKey?: (...params: Params) => string; - debugLabel?: string; -}): Cache { - const { debugLabel, getKey = defaultGetKey, load } = options; +const WearRefMapSymbol = Symbol.for("WearRefMap"); + +export function createCache, Value>( + options: CreateCacheOptions +): Cache { + const { config = {}, debugLabel, getKey = defaultGetKey, load } = options; + const { useWeakRef = true } = config; const debugLogInDev = (debug: string, params?: Params, ...args: any[]) => { if (DEBUG_LOG_IN_DEV && process.env.NODE_ENV === "development") { @@ -47,6 +58,10 @@ export function createCache, Value>(options: { const recordMap = new Map>(); const subscriberMap = new Map>(); + const weakRefMap = new WeakRefMap((cacheKey: string) => { + recordMap.delete(cacheKey); + subscriberMap.delete(cacheKey); + }); function abort(...params: Params): boolean { const cacheKey = getKey(...params); @@ -55,6 +70,7 @@ export function createCache, Value>(options: { debugLogInDev("abort()", params); recordMap.delete(cacheKey); + weakRefMap.delete(cacheKey); record.value.abortController.abort(); @@ -70,9 +86,11 @@ export function createCache, Value>(options: { const cacheKey = getKey(...params); const record: Record = { status: STATUS_RESOLVED, - value, + value: null, }; + writeRecordValue(record, value, ...params); + debugLogInDev("cache()", params, value); recordMap.set(cacheKey, record); @@ -83,6 +101,8 @@ export function createCache, Value>(options: { debugLogInDev(`evict()`, params); + weakRefMap.delete(cacheKey); + return recordMap.delete(cacheKey); } @@ -140,7 +160,7 @@ export function createCache, Value>(options: { } else if (record.status !== STATUS_RESOLVED) { throw Error(`Record found with status "${record.status}"`); } else { - return record.value; + return readRecordValue(record, ...params); } } @@ -149,7 +169,7 @@ export function createCache, Value>(options: { const record = recordMap.get(cacheKey); if (record?.status === STATUS_RESOLVED) { - return record.value; + return readRecordValue(record, ...params); } } @@ -165,7 +185,7 @@ export function createCache, Value>(options: { case STATUS_PENDING: return record.value.deferred; case STATUS_RESOLVED: - return record.value; + return readRecordValue(record, ...params); case STATUS_REJECTED: throw record.value; } @@ -174,7 +194,7 @@ export function createCache, Value>(options: { function fetchSuspense(...params: Params): Value { const record = getOrCreateRecord(...params); if (record.status === STATUS_RESOLVED) { - return record.value; + return readRecordValue(record, ...params); } else if (isPendingRecord(record)) { throw record.value.deferred; } else { @@ -210,7 +230,8 @@ export function createCache, Value>(options: { if (!abortSignal.aborted) { record.status = STATUS_RESOLVED; - record.value = value; + + writeRecordValue(record, value, ...params); deferred.resolve(value); } @@ -228,6 +249,16 @@ export function createCache, Value>(options: { } } + function readRecordValue(record: Record, ...params: Params): Value { + const value = record.value; + if (value === WearRefMapSymbol) { + const cacheKey = getKey(...params); + return weakRefMap.get(cacheKey); + } else { + return value; + } + } + function subscribeToStatus( callback: StatusCallback, ...params: Params @@ -255,6 +286,21 @@ export function createCache, Value>(options: { } } + function writeRecordValue( + record: Record, + value: Value, + ...params: Params + ): void { + if (useWeakRef && value != null && typeof value === "object") { + record.value = WearRefMapSymbol; + + const cacheKey = getKey(...params); + weakRefMap.set(cacheKey, value); + } else { + record.value = value; + } + } + return { abort, cache, diff --git a/packages/suspense/src/cache/createSingleEntryCache.ts b/packages/suspense/src/cache/createSingleEntryCache.ts index e036599..78c32c5 100644 --- a/packages/suspense/src/cache/createSingleEntryCache.ts +++ b/packages/suspense/src/cache/createSingleEntryCache.ts @@ -1,20 +1,16 @@ -import { createCache } from "./createCache"; -import { Cache, CacheLoadOptions, Thenable } from "../types"; +import { createCache, CreateCacheOptions } from "./createCache"; +import { Cache } from "../types"; const key = Symbol.for("createSingleEntryCache").toString(); -export function createSingleEntryCache< - Params extends Array, - Value ->(options: { - load: (...params: [...Params, CacheLoadOptions]) => Thenable | Value; - debugLabel?: string; -}): Cache { +export function createSingleEntryCache, Value>( + options: Omit, "getKey"> +): Cache { if (options.hasOwnProperty("getKey")) { throw Error("createSingleEntryCache does not support a getKey option"); } - return createCache({ + return createCache({ getKey: () => key, ...options, }); diff --git a/packages/suspense/src/utils/WeakRefMap.test.ts b/packages/suspense/src/utils/WeakRefMap.test.ts new file mode 100644 index 0000000..807e633 --- /dev/null +++ b/packages/suspense/src/utils/WeakRefMap.test.ts @@ -0,0 +1,39 @@ +import { WeakRefMap } from "./WeakRefMap"; + +describe("WeakRefMap", () => { + it("should implement a basic Map-like API", () => { + const finalizer = jest.fn(); + + const foo = { foo: true }; + const bar = { bar: true }; + + const map = new WeakRefMap(finalizer); + map.set("foo", foo); + map.set("bar", bar); + + expect(map.size()).toBe(2); + expect(map.has("foo")).toBe(true); + expect(map.has("bar")).toBe(true); + expect(map.get("foo")).toBe(foo); + expect(map.get("bar")).toBe(bar); + + expect(map.delete("foo")).toBe(true); + expect(map.size()).toBe(1); + expect(map.has("foo")).toBe(false); + expect(map.get("foo")).toBe(undefined); + expect(map.has("bar")).toBe(true); + expect(map.get("bar")).toBe(bar); + + map.set("bar", foo); + expect(map.size()).toBe(1); + expect(map.has("bar")).toBe(true); + expect(map.get("bar")).toBe(foo); + + map.delete("bar"); + expect(map.size()).toBe(0); + expect(map.has("foo")).toBe(false); + expect(map.has("bar")).toBe(false); + expect(map.get("foo")).toBe(undefined); + expect(map.get("bar")).toBe(undefined); + }); +}); diff --git a/packages/suspense/src/utils/WeakRefMap.ts b/packages/suspense/src/utils/WeakRefMap.ts new file mode 100644 index 0000000..afe3fd5 --- /dev/null +++ b/packages/suspense/src/utils/WeakRefMap.ts @@ -0,0 +1,52 @@ +export type FinalizerCallback = (key: Key) => void; + +export class WeakRefMap { + private finalizationRegistry: FinalizationRegistry; + private map: Map>; + + constructor(finalizerCallback: FinalizerCallback) { + this.finalizationRegistry = new FinalizationRegistry((key) => { + this.map.delete(key); + + finalizerCallback(key); + }); + this.map = new Map>(); + } + + delete(key: Key): boolean { + const weakRef = this.map.get(key); + + const result = this.map.delete(key); + + if (weakRef) { + const value = weakRef.deref(); + if (value != null) { + this.finalizationRegistry.unregister(value); + } + } + + return result; + } + + get(key: Key): Value | undefined { + const weakRef = this.map.get(key); + + return weakRef ? weakRef.deref() : undefined; + } + + has(key: Key): boolean { + return this.map.has(key); + } + + size(): number { + return this.map.size; + } + + set(key: Key, value: Value): void { + this.map.set(key, new WeakRef(value)); + + if (value != null) { + this.finalizationRegistry.register(value, key, value); + } + } +} diff --git a/packages/suspense/src/utils/stringify.ts b/packages/suspense/src/utils/stringify.ts new file mode 100644 index 0000000..f6508d1 --- /dev/null +++ b/packages/suspense/src/utils/stringify.ts @@ -0,0 +1,20 @@ +// Convenience function to JSON-stringify values that may contain circular references. +export function stringify(value: any, space?: string | number): string { + const cache: any[] = []; + + return JSON.stringify( + value, + (key, value) => { + if (typeof value === "object" && value !== null) { + if (cache.includes(value)) { + return "[Circular]"; + } + + cache.push(value); + } + + return value; + }, + space + ); +} diff --git a/tsconfig.json b/tsconfig.json index b1c6651..d1a1681 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -1,7 +1,7 @@ { "compilerOptions": { "esModuleInterop": true, - "lib": ["ES2015", "DOM"], + "lib": ["DOM", "ES2015", "ES2021.WeakRef"], "module": "es2020", "moduleResolution": "node", "noImplicitAny": true, From b5807ec1933d40dc906f7d7142b04a6208f7aa6b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 28 Feb 2023 08:56:19 -0500 Subject: [PATCH 2/4] Add WeakRefMap GC test --- .../workflows/{jest.yml => jest.browser.yml} | 2 +- .github/workflows/jest.node.yml | 15 ++++ packages/suspense/jest.config.node.js | 5 ++ packages/suspense/package.json | 2 + .../src/cache/createCache.test.node.ts | 80 +++++++++++++++++++ ...RefMap.test.ts => WeakRefMap.test.node.ts} | 26 +++++- packages/suspense/src/utils/test.ts | 22 +++++ 7 files changed, 148 insertions(+), 4 deletions(-) rename .github/workflows/{jest.yml => jest.browser.yml} (89%) create mode 100644 .github/workflows/jest.node.yml create mode 100644 packages/suspense/jest.config.node.js create mode 100644 packages/suspense/src/cache/createCache.test.node.ts rename packages/suspense/src/utils/{WeakRefMap.test.ts => WeakRefMap.test.node.ts} (62%) create mode 100644 packages/suspense/src/utils/test.ts diff --git a/.github/workflows/jest.yml b/.github/workflows/jest.browser.yml similarity index 89% rename from .github/workflows/jest.yml rename to .github/workflows/jest.browser.yml index cde522e..418259e 100644 --- a/.github/workflows/jest.yml +++ b/.github/workflows/jest.browser.yml @@ -2,7 +2,7 @@ name: "Jest unit tests" on: [pull_request] jobs: tests_e2e: - name: Run unit tests with Jest + name: Run Jest (browser based) unit tests runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 diff --git a/.github/workflows/jest.node.yml b/.github/workflows/jest.node.yml new file mode 100644 index 0000000..470e5da --- /dev/null +++ b/.github/workflows/jest.node.yml @@ -0,0 +1,15 @@ +name: "Jest unit tests" +on: [pull_request] +jobs: + tests_e2e: + name: Run Jest (node based) unit tests + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + - uses: actions/setup-node@v3 + - name: Initialize Yarn and install package dependencies + run: yarn + - name: Build NPM package + run: yarn prerelease + - name: Run tests + run: cd packages/suspense && yarn test:node \ No newline at end of file diff --git a/packages/suspense/jest.config.node.js b/packages/suspense/jest.config.node.js new file mode 100644 index 0000000..ce2aff1 --- /dev/null +++ b/packages/suspense/jest.config.node.js @@ -0,0 +1,5 @@ +/** @type {import('ts-jest').JestConfigWithTsJest} */ +module.exports = { + preset: "ts-jest", + testMatch: ["**/*.test.node.{ts,tsx}"], +}; diff --git a/packages/suspense/package.json b/packages/suspense/package.json index d2e4de5..5000e0e 100644 --- a/packages/suspense/package.json +++ b/packages/suspense/package.json @@ -15,6 +15,8 @@ "scripts": { "build": "parcel build", "test": "jest", + "test:node": "node --expose-gc node_modules/.bin/jest --config=jest.config.node.js", + "test:node:watch": "node --expose-gc node_modules/.bin/jest --config=jest.config.node.js --watch", "test:watch": "jest --watch", "watch": "parcel watch" }, diff --git a/packages/suspense/src/cache/createCache.test.node.ts b/packages/suspense/src/cache/createCache.test.node.ts new file mode 100644 index 0000000..0a66636 --- /dev/null +++ b/packages/suspense/src/cache/createCache.test.node.ts @@ -0,0 +1,80 @@ +import { createCache } from "./createCache"; +import { Cache, CacheLoadOptions } from "../types"; +import { WeakRefMap } from "../utils/WeakRefMap"; +import { requestGC, waitForGC } from "../utils/test"; + +describe("createCache", () => { + let cache: Cache<[string], Object>; + let fetch: jest.Mock | Object, [string, CacheLoadOptions]>; + + afterEach(async () => { + await runAndWaitForGC(); + }); + + beforeEach(() => { + fetch = jest.fn(); + fetch.mockImplementation((key: string) => { + if (key.startsWith("async")) { + return Promise.resolve(key); + } else if (key.startsWith("error")) { + return Promise.reject(key); + } else { + return key; + } + }); + }); + + it("should use WeakRefs if requested", async () => { + cache = createCache<[string], Object>({ + config: { useWeakRef: true }, + load: fetch, + }); + + cache.cache(createObject(), "one"); + cache.cache(createObject(), "two"); + cache.cache(createObject(), "three"); + + expect(cache.getValueIfCached("one")).not.toBeUndefined(); + expect(cache.getValueIfCached("two")).not.toBeUndefined(); + expect(cache.getValueIfCached("three")).not.toBeUndefined(); + + await runAndWaitForGC(); + + expect(cache.getValueIfCached("one")).toBeUndefined(); + expect(cache.getValueIfCached("two")).toBeUndefined(); + expect(cache.getValueIfCached("three")).toBeUndefined(); + }); + + it("should not use WeakRefs if requested", async () => { + cache = createCache<[string], Object>({ + config: { useWeakRef: false }, + load: fetch, + }); + + cache.cache(createObject(), "one"); + cache.cache(createObject(), "two"); + + expect(cache.getValueIfCached("one")).not.toBeUndefined(); + expect(cache.getValueIfCached("two")).not.toBeUndefined(); + + await runAndWaitForGC(); + + expect(cache.getValueIfCached("one")).not.toBeUndefined(); + expect(cache.getValueIfCached("two")).not.toBeUndefined(); + }); +}); + +function createObject(): Object { + return {}; +} + +async function runAndWaitForGC() { + const finalizer = jest.fn(); + + const map = new WeakRefMap(finalizer); + map.set("control", createObject()); + + requestGC(); + + await waitForGC(() => finalizer.mock.calls.length > 0); +} diff --git a/packages/suspense/src/utils/WeakRefMap.test.ts b/packages/suspense/src/utils/WeakRefMap.test.node.ts similarity index 62% rename from packages/suspense/src/utils/WeakRefMap.test.ts rename to packages/suspense/src/utils/WeakRefMap.test.node.ts index 807e633..b21da23 100644 --- a/packages/suspense/src/utils/WeakRefMap.test.ts +++ b/packages/suspense/src/utils/WeakRefMap.test.node.ts @@ -1,13 +1,19 @@ +import { requestGC, waitForGC } from "./test"; import { WeakRefMap } from "./WeakRefMap"; describe("WeakRefMap", () => { - it("should implement a basic Map-like API", () => { - const finalizer = jest.fn(); + let finalizer: jest.Mock>; + let map: WeakRefMap; + + beforeEach(() => { + finalizer = jest.fn(); + map = new WeakRefMap(finalizer); + }); + it("should implement a basic Map-like API", () => { const foo = { foo: true }; const bar = { bar: true }; - const map = new WeakRefMap(finalizer); map.set("foo", foo); map.set("bar", bar); @@ -36,4 +42,18 @@ describe("WeakRefMap", () => { expect(map.get("foo")).toBe(undefined); expect(map.get("bar")).toBe(undefined); }); + + it("should call the finalizer function when a value is garbage collected", async () => { + map.set("foo", { foo: true }); + map.set("bar", { bar: true }); + + finalizer.mockReset(); + + requestGC(); + + await waitForGC(() => finalizer.mock.calls.length === 2); + + expect(finalizer).toHaveBeenCalledWith("foo"); + expect(finalizer).toHaveBeenCalledWith("bar"); + }); }); diff --git a/packages/suspense/src/utils/test.ts b/packages/suspense/src/utils/test.ts new file mode 100644 index 0000000..dfbc7c2 --- /dev/null +++ b/packages/suspense/src/utils/test.ts @@ -0,0 +1,22 @@ +export function requestGC(): void { + // Node --expose-gc flag + global.gc(); +} + +export async function wait(delay: number): Promise { + await new Promise((resolve) => setTimeout(resolve, delay)); +} + +export async function waitForGC( + condition: () => boolean, + timeoutInSeconds: number = 5 +): Promise { + const startTime = process.hrtime(); + do { + global.gc(); + await wait(0); + } while ( + condition() === false && + process.hrtime(startTime)[0] < timeoutInSeconds + ); +} From 6bdfb5b3ba5e440577ffb4d402762fabf70b201f Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 28 Feb 2023 15:30:50 -0500 Subject: [PATCH 3/4] WeakRefMap timing and key conflict value edge case handling --- .../src/cache/createCache.test.node.ts | 22 ++------ .../src/utils/WeakRefMap.test.node.ts | 24 ++++++--- packages/suspense/src/utils/WeakRefMap.ts | 31 ++++++++---- packages/suspense/src/utils/test.ts | 50 +++++++++++++++---- 4 files changed, 82 insertions(+), 45 deletions(-) diff --git a/packages/suspense/src/cache/createCache.test.node.ts b/packages/suspense/src/cache/createCache.test.node.ts index 0a66636..eb7b5e1 100644 --- a/packages/suspense/src/cache/createCache.test.node.ts +++ b/packages/suspense/src/cache/createCache.test.node.ts @@ -1,16 +1,11 @@ import { createCache } from "./createCache"; import { Cache, CacheLoadOptions } from "../types"; -import { WeakRefMap } from "../utils/WeakRefMap"; import { requestGC, waitForGC } from "../utils/test"; describe("createCache", () => { let cache: Cache<[string], Object>; let fetch: jest.Mock | Object, [string, CacheLoadOptions]>; - afterEach(async () => { - await runAndWaitForGC(); - }); - beforeEach(() => { fetch = jest.fn(); fetch.mockImplementation((key: string) => { @@ -38,7 +33,8 @@ describe("createCache", () => { expect(cache.getValueIfCached("two")).not.toBeUndefined(); expect(cache.getValueIfCached("three")).not.toBeUndefined(); - await runAndWaitForGC(); + await requestGC(); + await waitForGC(); expect(cache.getValueIfCached("one")).toBeUndefined(); expect(cache.getValueIfCached("two")).toBeUndefined(); @@ -57,7 +53,8 @@ describe("createCache", () => { expect(cache.getValueIfCached("one")).not.toBeUndefined(); expect(cache.getValueIfCached("two")).not.toBeUndefined(); - await runAndWaitForGC(); + await requestGC(); + await waitForGC(); expect(cache.getValueIfCached("one")).not.toBeUndefined(); expect(cache.getValueIfCached("two")).not.toBeUndefined(); @@ -67,14 +64,3 @@ describe("createCache", () => { function createObject(): Object { return {}; } - -async function runAndWaitForGC() { - const finalizer = jest.fn(); - - const map = new WeakRefMap(finalizer); - map.set("control", createObject()); - - requestGC(); - - await waitForGC(() => finalizer.mock.calls.length > 0); -} diff --git a/packages/suspense/src/utils/WeakRefMap.test.node.ts b/packages/suspense/src/utils/WeakRefMap.test.node.ts index b21da23..d36f663 100644 --- a/packages/suspense/src/utils/WeakRefMap.test.node.ts +++ b/packages/suspense/src/utils/WeakRefMap.test.node.ts @@ -17,26 +17,22 @@ describe("WeakRefMap", () => { map.set("foo", foo); map.set("bar", bar); - expect(map.size()).toBe(2); expect(map.has("foo")).toBe(true); expect(map.has("bar")).toBe(true); expect(map.get("foo")).toBe(foo); expect(map.get("bar")).toBe(bar); expect(map.delete("foo")).toBe(true); - expect(map.size()).toBe(1); expect(map.has("foo")).toBe(false); expect(map.get("foo")).toBe(undefined); expect(map.has("bar")).toBe(true); expect(map.get("bar")).toBe(bar); map.set("bar", foo); - expect(map.size()).toBe(1); expect(map.has("bar")).toBe(true); expect(map.get("bar")).toBe(foo); map.delete("bar"); - expect(map.size()).toBe(0); expect(map.has("foo")).toBe(false); expect(map.has("bar")).toBe(false); expect(map.get("foo")).toBe(undefined); @@ -49,11 +45,27 @@ describe("WeakRefMap", () => { finalizer.mockReset(); - requestGC(); - + await requestGC(); await waitForGC(() => finalizer.mock.calls.length === 2); expect(finalizer).toHaveBeenCalledWith("foo"); expect(finalizer).toHaveBeenCalledWith("bar"); }); + + it("should unregister a value when a key is updated", async () => { + map.set("test", { label: "one" }); + + expect(finalizer).not.toHaveBeenCalled(); + + const value = { label: "two" }; + map.set("test", value); + + await requestGC(); + await waitForGC(() => finalizer.mock.calls.length > 0); + + expect(finalizer).toHaveBeenCalledWith("test"); + + expect(map.has("test")).toBe(true); + expect(map.get("test")).toBe(value); + }); }); diff --git a/packages/suspense/src/utils/WeakRefMap.ts b/packages/suspense/src/utils/WeakRefMap.ts index afe3fd5..7139b5a 100644 --- a/packages/suspense/src/utils/WeakRefMap.ts +++ b/packages/suspense/src/utils/WeakRefMap.ts @@ -1,29 +1,35 @@ export type FinalizerCallback = (key: Key) => void; export class WeakRefMap { + private finalizerCallback: FinalizerCallback; private finalizationRegistry: FinalizationRegistry; private map: Map>; constructor(finalizerCallback: FinalizerCallback) { + this.finalizerCallback = finalizerCallback; + + this.map = new Map>(); this.finalizationRegistry = new FinalizationRegistry((key) => { this.map.delete(key); finalizerCallback(key); }); - this.map = new Map>(); } - delete(key: Key): boolean { + private unregister(key: Key): void { const weakRef = this.map.get(key); - - const result = this.map.delete(key); - if (weakRef) { const value = weakRef.deref(); if (value != null) { this.finalizationRegistry.unregister(value); } } + } + + delete(key: Key): boolean { + const result = this.map.delete(key); + + this.unregister(key); return result; } @@ -35,14 +41,19 @@ export class WeakRefMap { } has(key: Key): boolean { - return this.map.has(key); - } - - size(): number { - return this.map.size; + // Handle timing edge case in case value has been GC'ed + // but FinalizationRegistry callback has not yet run. + return this.map.has(key) && this.map.get(key).deref() != null; } set(key: Key, value: Value): void { + if (this.map.has(key)) { + this.unregister(key); + + // FinalizationRegistry won't trigger if we unregister. + this.finalizerCallback(key); + } + this.map.set(key, new WeakRef(value)); if (value != null) { diff --git a/packages/suspense/src/utils/test.ts b/packages/suspense/src/utils/test.ts index dfbc7c2..4310866 100644 --- a/packages/suspense/src/utils/test.ts +++ b/packages/suspense/src/utils/test.ts @@ -1,22 +1,50 @@ -export function requestGC(): void { +export async function requestGC() { + await wait(100); + // Node --expose-gc flag global.gc(); + + await wait(100); } export async function wait(delay: number): Promise { await new Promise((resolve) => setTimeout(resolve, delay)); } +export async function waitUntil( + conditional: () => boolean, + timeout: number = 5000 +) { + let startTime = performance.now(); + do { + if (conditional()) { + return; + } + + await wait(100); + } while (startTime + timeout > performance.now()); + + const elapsed = performance.now() - startTime; + + throw Error(`Condition not met within ${elapsed}ms`); +} + export async function waitForGC( - condition: () => boolean, - timeoutInSeconds: number = 5 + conditional: () => boolean = () => true, + timeout: number = 5000 ): Promise { - const startTime = process.hrtime(); - do { - global.gc(); - await wait(0); - } while ( - condition() === false && - process.hrtime(startTime)[0] < timeoutInSeconds - ); + const finalizer = jest.fn(); + const finalizationRegistry = new FinalizationRegistry(finalizer); + finalizationRegistry.register({}, "control"); + + await requestGC(); + + // Ensure some GC has occurred + await waitUntil(() => finalizer.mock.calls.length > 0); + + expect(finalizer).toHaveBeenCalledTimes(1); + expect(finalizer).toHaveBeenCalledWith("control"); + + // Ensure additional constraints are met + await waitUntil(conditional, timeout); } From 8abaa514011b0c8725ac07772176aa355aaf1ec5 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 28 Feb 2023 15:51:16 -0500 Subject: [PATCH 4/4] Added useWeakRef flag to docs --- packages/suspense-website/index.tsx | 22 ++++++--- .../src/components/Note.module.css | 11 +++++ .../createCache/cacheWithoutWeakRef.ts | 11 +++++ .../suspense-website/src/examples/index.ts | 6 +++ packages/suspense-website/src/routes/Home.tsx | 18 ++++--- .../src/routes/api/createCache.tsx | 11 ++++- .../suspense-website/src/routes/config.ts | 12 +++-- .../src/routes/examples/memory-management.tsx | 49 +++++++++++++++++++ 8 files changed, 120 insertions(+), 20 deletions(-) create mode 100644 packages/suspense-website/src/examples/createCache/cacheWithoutWeakRef.ts create mode 100644 packages/suspense-website/src/routes/examples/memory-management.tsx diff --git a/packages/suspense-website/index.tsx b/packages/suspense-website/index.tsx index 3ae4639..6c58a7b 100644 --- a/packages/suspense-website/index.tsx +++ b/packages/suspense-website/index.tsx @@ -6,10 +6,11 @@ import { CREATE_DEFERRED, CREATE_SINGLE_ENTRY_CACHE, CREATE_STREAMING_CACHE, - EXAMPLE_ABORT_A_REQUEST, - EXAMPLE_FETCH_WITH_STATUS, - EXAMPLE_MUTATING_A_CACHE_VALUE, - EXAMPLE_STREAMING_CACHE, + GUIDE_ABORT_A_REQUEST, + GUIDE_FETCH_WITH_STATUS, + GUIDE_MEMORY_MANAGEMENT, + GUIDE_MUTATING_A_CACHE_VALUE, + GUIDE_STREAMING_CACHE, IS_THENNABLE, USE_CACHE_STATUS, USE_STREAMING_CACHE, @@ -25,6 +26,7 @@ import PageNotFoundRoute from "./src/routes/PageNotFound"; import UseCacheStatusRoute from "./src/routes/api/useCacheStatus"; import UseStreamingValuesRoute from "./src/routes/api/useStreamingValues"; import AbortingRequestRoute from "./src/routes/examples/aborting-a-request"; +import MemoryManagementRoute from "./src/routes/examples/memory-management"; import MutatingCacheValueRoute from "./src/routes/examples/mutating-a-cache-value"; import RenderingStatusWhileFetchingRoute from "./src/routes/examples/rendering-status-while-fetching"; import CreatingStreamingCacheRoute from "./src/routes/examples/streaming-cache"; @@ -51,19 +53,23 @@ root.render( element={} /> } /> } /> } + /> + } /> } /> } /> diff --git a/packages/suspense-website/src/components/Note.module.css b/packages/suspense-website/src/components/Note.module.css index 076e389..ed4ba24 100644 --- a/packages/suspense-website/src/components/Note.module.css +++ b/packages/suspense-website/src/components/Note.module.css @@ -55,6 +55,17 @@ flex: 0 0 1.75rem !important; } +.Content { + display: flex; + flex-direction: column; + gap: 0.5rem; +} + +.Content * { + line-height: 1; + margin: 0; +} + .Note[data-type="note"] .Content, .Note[data-type="quote"] .Content { line-height: 1rem; diff --git a/packages/suspense-website/src/examples/createCache/cacheWithoutWeakRef.ts b/packages/suspense-website/src/examples/createCache/cacheWithoutWeakRef.ts new file mode 100644 index 0000000..c27f827 --- /dev/null +++ b/packages/suspense-website/src/examples/createCache/cacheWithoutWeakRef.ts @@ -0,0 +1,11 @@ +import { CacheLoadOptions, createCache } from "suspense"; + +const load = async () => null as any; + +// REMOVE_BEFORE + +createCache<[userId: string], JSON>({ + config: { useWeakRef: false }, + load, + // ... +}); diff --git a/packages/suspense-website/src/examples/index.ts b/packages/suspense-website/src/examples/index.ts index 368d8a7..70f804d 100644 --- a/packages/suspense-website/src/examples/index.ts +++ b/packages/suspense-website/src/examples/index.ts @@ -31,6 +31,12 @@ const createCache = { cacheWithSignal: processExample( readFileSync(join(__dirname, "createCache", "cacheWithSignal.ts"), "utf8") ), + cacheWithoutWeakRef: processExample( + readFileSync( + join(__dirname, "createCache", "cacheWithoutWeakRef.ts"), + "utf8" + ) + ), evict: processExample( readFileSync(join(__dirname, "createCache", "evict.ts"), "utf8") ), diff --git a/packages/suspense-website/src/routes/Home.tsx b/packages/suspense-website/src/routes/Home.tsx index 83a97b8..1e6fa4b 100644 --- a/packages/suspense-website/src/routes/Home.tsx +++ b/packages/suspense-website/src/routes/Home.tsx @@ -10,9 +10,10 @@ import { CREATE_DEFERRED, CREATE_SINGLE_ENTRY_CACHE, CREATE_STREAMING_CACHE, - EXAMPLE_ABORT_A_REQUEST, - EXAMPLE_FETCH_WITH_STATUS, - EXAMPLE_STREAMING_CACHE, + GUIDE_ABORT_A_REQUEST, + GUIDE_FETCH_WITH_STATUS, + GUIDE_MEMORY_MANAGEMENT, + GUIDE_STREAMING_CACHE, IS_THENNABLE, USE_CACHE_STATUS, USE_STREAMING_CACHE, @@ -93,19 +94,24 @@ export default function Route() {
    +
diff --git a/packages/suspense-website/src/routes/api/createCache.tsx b/packages/suspense-website/src/routes/api/createCache.tsx index 12a7cc9..b8b2ed9 100644 --- a/packages/suspense-website/src/routes/api/createCache.tsx +++ b/packages/suspense-website/src/routes/api/createCache.tsx @@ -5,7 +5,11 @@ import { createCache } from "../../examples"; import Header from "../../components/Header"; import SubHeading from "../../components/SubHeading"; import { Link } from "react-router-dom"; -import { IS_THENNABLE, USE_CACHE_STATUS } from "../config"; +import { + GUIDE_MEMORY_MANAGEMENT, + IS_THENNABLE, + USE_CACHE_STATUS, +} from "../config"; import Note from "../../components/Note"; import { ExternalLink } from "../../components/ExternalLink"; @@ -48,6 +52,11 @@ export default function Route() { that's provided to support cancellation.

+ + Caches use WeakRef and FinalizationRegistry{" "} + by default,{" "} + but this is configurable. + diff --git a/packages/suspense-website/src/routes/config.ts b/packages/suspense-website/src/routes/config.ts index 408b733..9ad14f4 100644 --- a/packages/suspense-website/src/routes/config.ts +++ b/packages/suspense-website/src/routes/config.ts @@ -1,3 +1,4 @@ +// API export const CREATE_CACHE = "/createCache"; export const CREATE_DEFERRED = "/createDeferred"; export const CREATE_SINGLE_ENTRY_CACHE = "/createSingleEntryCache"; @@ -6,8 +7,9 @@ export const IS_THENNABLE = "/isThenable"; export const USE_CACHE_STATUS = "/useCacheStatus"; export const USE_STREAMING_CACHE = "/useStreamingValues"; -// Examples -export const EXAMPLE_ABORT_A_REQUEST = "/example/abort-a-request"; -export const EXAMPLE_FETCH_WITH_STATUS = "/example/rendering-cache-status"; -export const EXAMPLE_MUTATING_A_CACHE_VALUE = "/example/mutating-a-cache-value"; -export const EXAMPLE_STREAMING_CACHE = "/example/writing-a-streaming-cache"; +// Guides +export const GUIDE_ABORT_A_REQUEST = "/guide/abort-a-request"; +export const GUIDE_FETCH_WITH_STATUS = "/guide/rendering-cache-status"; +export const GUIDE_MEMORY_MANAGEMENT = "/guide/memory-management"; +export const GUIDE_MUTATING_A_CACHE_VALUE = "/guide/mutating-a-cache-value"; +export const GUIDE_STREAMING_CACHE = "/guide/writing-a-streaming-cache"; diff --git a/packages/suspense-website/src/routes/examples/memory-management.tsx b/packages/suspense-website/src/routes/examples/memory-management.tsx new file mode 100644 index 0000000..7775774 --- /dev/null +++ b/packages/suspense-website/src/routes/examples/memory-management.tsx @@ -0,0 +1,49 @@ +import { Link } from "react-router-dom"; +import Block from "../../components/Block"; +import Code from "../../components/Code"; +import Container from "../../components/Container"; +import { ExternalLink } from "../../components/ExternalLink"; +import Header from "../../components/Header"; +import Note from "../../components/Note"; +import { createCache } from "../../examples"; +import { CREATE_CACHE } from "../config"; + +export default function Route() { + return ( + + +
+ + +

+ Caches created with{" "} + + createCache + {" "} + use{" "} + + + WeakRef + + {" "} + and{" "} + + + FinalizationRegistry + + {" "} + APIs to avoid leaking memory. This behavior can be disabled using the{" "} + useWeakRef configuration flag. +

+ + +

Caches that don't use weak refs may leak memory over time.

+

+ To avoid this, use the evict method to remove entries + once you are done using them. +

+ + + + ); +}