Skip to content

fix(subscription): delete bookkeeping Map entries on last unmount#4262

Draft
spokodev wants to merge 1 commit into
vercel:mainfrom
spokodev:fix/subscription-cleanup-maps-on-teardown
Draft

fix(subscription): delete bookkeeping Map entries on last unmount#4262
spokodev wants to merge 1 commit into
vercel:mainfrom
spokodev:fix/subscription-cleanup-maps-on-teardown

Conversation

@spokodev
Copy link
Copy Markdown

Fixes #4261.

The cleanup callback at src/subscription/index.ts:84-93 calls dispose() when the last subscriber for a key unmounts, but never removes the matching entries from the subscriptions and disposers Maps. In a long-lived app that cycles through many distinct subscription keys (paginated feeds, rotating live-data channels, user-navigated rooms), both Maps accumulate one dead entry per unique key and grow without bound.

This change deletes the entry from both Maps right after calling dispose():

if (!count) {
  const dispose = disposers.get(subscriptionKey)
  dispose?.()
  subscriptions.delete(subscriptionKey)
  disposers.delete(subscriptionKey)
}

The functional behaviour is unchanged: a subsequent re-mount for the same key still hits the if (!refCount) branch (because subscriptions.get(key) || 0 is 0 whether the entry is missing or zero), so a fresh subscribe runs and a fresh disposer is registered.

Tests

test/use-swr-subscription.test.tsx gets a teardown cleans up bookkeeping Maps (#4261) describe block with 7 new cases covering both axes of the fix.

Because the bookkeeping Maps are module-private (closed over inside subscription/index.ts via a per-cache WeakMap), a minimal @internal helper is exported so the tests can read their sizes:

export const _unstable_getSubscriptionStateSize = (
  cache: object
): { subscriptions: number; disposers: number } | null

The naming follows the standard _unstable_ prefix convention and the helper is annotated @internal so it does not enter the documented public API. Suggestions on a better location welcome.

Positive (the fix's contract)

  • After the last subscriber unmounts, both Maps drop the entry (sizes go to 0).

Negative (existing behavior preserved)

  • While another subscriber is still mounted, the entry stays in both Maps (refcount > 0).
  • dispose() is still called exactly once on the final unmount.
  • Re-mounting the same key after teardown runs a fresh subscribe and produces a fresh disposer.

Edge

  • Distinct keys are isolated. Unmounting one does not affect another active subscription on the same cache.
  • N mount/unmount cycles on distinct keys (the leak scenario from the issue) leave the Maps at size 0.
  • Per-cache scope. Cleaning up one SWRConfig's subscription does not touch the bookkeeping of another SWRConfig.

All 7 fail on main against the relevant assertions and pass on this branch. The 7 pre-existing tests in the same file are unchanged and still pass.

Verification

pnpm test
# 367 passed, 5 skipped (372)

pnpm test test/use-swr-subscription.test.tsx
# 14 passed

pnpm types:check
# clean

pnpm exec eslint src/subscription/index.ts test/use-swr-subscription.test.tsx
# clean

pnpm prettier --check src/subscription/index.ts test/use-swr-subscription.test.tsx
# All matched files use Prettier code style!

Opened as draft until CI is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

subscription/index.ts L89-92: disposers and subscriptions Maps not cleared on last-subscriber teardown

1 participant