fix potential memory leak in `Concast`, add tests #11358

phryneas · 2023-11-10T11:19:37Z

This adds a bunch of memory-related tests to Concast and also fixes two scenarios (that were not triggered in our codebase) where an actual memory leak would be created.

You can see the "before->after" in this commit.

Checklist:

If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests

changeset-bot · 2023-11-10T11:19:41Z

🦋 Changeset detected

Latest commit: 1bbd815

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-11-10T12:03:33Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	37.73 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	44.2 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	42.68 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	32.79 KB (+0.01% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	31.46 KB (+0.01% 🔺)
import { ApolloProvider } from "dist/react/index.js"	1.28 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.26 KB (0%)
import { useQuery } from "dist/react/index.js"	4.38 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.19 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	4.69 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.51 KB (0%)
import { useMutation } from "dist/react/index.js"	2.65 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.63 KB (0%)
import { useSubscription } from "dist/react/index.js"	2.34 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.29 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	4.37 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	3.79 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	3.86 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.27 KB (0%)
import { useLoadableQuery } from "dist/react/index.js"	4.12 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.53 KB (0%)
import { useReadQuery } from "dist/react/index.js"	3.05 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	3 KB (0%)
import { useFragment } from "dist/react/index.js"	2.15 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.1 KB (0%)

phryneas · 2023-11-10T12:49:18Z

The tests are currently failing because they are not part of the tsconfig.json and we need to manually add WeakMap there, so I'm leaving this in draft status until we got the big TS PR and #11359 merged.

…oryLeaks

alessbell · 2023-11-29T15:13:48Z

.changeset/forty-cups-shop.md

+"@apollo/client": patch
+---
+
+Fixes a potential memory leak (that was not triggered by ApolloClient until now) in `Concast`.


Additional context: this leak never occurred in our runtime code, but could present a problem for different usages of Concast (that do not appear in our codebase). Since Concast is exported from our library, we should fix it as good citizens.

@alessbell that additional context is super helpful as I was a bit confused at what the "until now" meant.

@phryneas perhaps you could word this as such to make it more clear?

Fixes a potential memory leak in Concast when Concast was used outside of Apollo Client.

This gives a bit better context on when someone could expect to see the issue arise.

alessbell · 2023-11-29T15:15:37Z

src/testing/matchers/index.d.ts

@@ -10,6 +10,11 @@ import {
  ProfiledHook,
 } from "../internal/index.js";

+declare class WeakRef<T extends WeakKey> {


We can remove this since we're including it in our tsconfig.

alessbell · 2023-11-29T15:16:07Z

src/testing/matchers/toBeGarbageCollected.ts

@@ -0,0 +1,57 @@
+import type { MatcherFunction } from "expect";
+
+declare class WeakRef<T extends WeakKey> {


We can also remove this declaration since we're including it in our tsconfig.

alessbell · 2023-11-29T15:19:36Z

src/tsconfig.json

@@ -5,7 +5,7 @@
 {
  "compilerOptions": {
    "noEmit": true,
-    "lib": ["es2015", "esnext.asynciterable", "dom"],
+    "lib": ["es2015", "esnext.asynciterable", "dom", "ES2021.WeakRef"],


With our two tsconfigs now, the one extending the outer config which is used for builds, this change will not display an error in our editors if we use WeakRefs but tsc would fail at build time.

alessbell · 2023-11-29T15:26:06Z

src/testing/matchers/toBeGarbageCollected.ts

+    return {
+      pass,
+      message: () => {
+        if (pass) {


Note on how matchers work: if the user is expecting a pass and pass is true, no message is shown. So the two return cases here cover if the user gets a result that is unexpected, both in the affirmative and negative.

alessbell · 2023-11-29T15:29:11Z

src/utilities/observables/__tests__/Concast.ts

 });
+
+function deferred<X>() {
+  let resolve!: (v: X) => void, reject!: (e: any) => void;


Minor nit: declaring variables on separate lines for readability

Don't know why prettier let me down here ^^

alessbell · 2023-11-29T15:38:47Z

src/utilities/observables/__tests__/Concast.ts

+    const concast = new Concast<number>([
+      Observable.of(1, 2),
+      promise,
+      Observable.of(3, 5),


It may not be load-bearing in this test, but initializing our Concast with a third item after our promise-wrapped Observable which will be rejected gives us additional signal that the promise rejects with an error even though there are additional observables coming after it.

alessbell · 2023-11-29T15:45:17Z

src/utilities/observables/__tests__/Concast.ts

+  });
+
+  it("rejecting a source of a concast frees all observer references on `this.observers`", async () => {
+    const observers: Observer<any>[] = [{ next() {}, error() {} }];


Suggested change

const observers: Observer<any>[] = [{ next() {}, error() {} }];

const subscribingObservers: Observer<any>[] = [{ next() {}, error() {} }];

alessbell · 2023-11-29T15:45:27Z

src/utilities/observables/__tests__/Concast.ts

+    const observers: Observer<any>[] = [{ next() {}, error() {} }];
+    const observerRefs = observers.map((observer) => new WeakRef(observer));
+
+    let observer!: Observer<number>;


Suggested change

let observer!: Observer<number>;

let sourceObserver!: Observer<number>;

alessbell

Just went through this together - thanks for walking through it! A few small comments in the course of our pairing but LGTM ✅

add tests for Concast, show memory leak

0f799d8

phryneas added this to the MemoryAnalysis milestone Nov 10, 2023

reset matcher-type-related changes

b25f1ef

phryneas added 5 commits November 10, 2023 13:03

add fix

2abfbf5

un-rename file

f135a79

remove comment

aff2af5

prettier

c6383c2

changeset

fb9632a

phryneas added 4 commits November 14, 2023 13:11

Merge branch 'release-3.9' into pr/concast-memoryLeaks

c492600

allow WeakRef to be used in tests

727bd44

size-limit

d98a5e9

Merge remote-tracking branch 'origin/release-3.9' into pr/concast-mem…

80d8b35

…oryLeaks

phryneas marked this pull request as ready for review November 14, 2023 14:21

phryneas requested review from jerelmiller and alessbell November 14, 2023 14:21

phryneas mentioned this pull request Nov 17, 2023

Persisted Query Link: improve memory management #11369

Merged

3 tasks

phryneas added 5 commits November 17, 2023 16:23

Merge remote-tracking branch 'origin/release-3.9' into pr/concast-mem…

4a8d622

…oryLeaks

update size-limit

6659ec4

Merge branch 'release-3.9' into pr/concast-memoryLeaks

75731f6

size-limit

606e5d6

better cleanup of interval/timeout

5c35069

alessbell reviewed Nov 29, 2023

View reviewed changes

alessbell approved these changes Nov 29, 2023

View reviewed changes

github-actions bot added the auto-cleanup 🤖 label Nov 29, 2023

phryneas added 3 commits November 30, 2023 15:25

review feedback

2992f11

add clarifying comment

59fe42c

re-add declaration

1bbd815

phryneas merged commit 7d939f8 into release-3.9 Nov 30, 2023
28 checks passed

phryneas deleted the pr/concast-memoryLeaks branch November 30, 2023 14:52

github-actions bot mentioned this pull request Nov 30, 2023

Version Packages (alpha) #11353

Merged

github-actions bot locked as resolved and limited conversation to collaborators Dec 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix potential memory leak in `Concast`, add tests #11358

fix potential memory leak in `Concast`, add tests #11358

phryneas commented Nov 10, 2023 •

edited

Loading

changeset-bot bot commented Nov 10, 2023 •

edited

Loading

github-actions bot commented Nov 10, 2023 •

edited

Loading

phryneas commented Nov 10, 2023 •

edited

Loading

alessbell Nov 29, 2023

jerelmiller Nov 29, 2023

alessbell Nov 29, 2023

alessbell Nov 29, 2023

alessbell Nov 29, 2023

alessbell Nov 29, 2023

alessbell Nov 29, 2023

phryneas Nov 30, 2023

alessbell Nov 29, 2023

alessbell Nov 29, 2023

alessbell Nov 29, 2023

alessbell left a comment

		@@ -0,0 +1,57 @@
		import type { MatcherFunction } from "expect";

		declare class WeakRef<T extends WeakKey> {

	const observers: Observer<any>[] = [{ next() {}, error() {} }];
	const subscribingObservers: Observer<any>[] = [{ next() {}, error() {} }];

	let observer!: Observer<number>;
	let sourceObserver!: Observer<number>;

fix potential memory leak in Concast, add tests #11358

fix potential memory leak in Concast, add tests #11358

Conversation

phryneas commented Nov 10, 2023 • edited Loading

Checklist:

changeset-bot bot commented Nov 10, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Nov 10, 2023 • edited Loading

size-limit report 📦

phryneas commented Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessbell left a comment

Choose a reason for hiding this comment

fix potential memory leak in `Concast`, add tests #11358

fix potential memory leak in `Concast`, add tests #11358

phryneas commented Nov 10, 2023 •

edited

Loading

changeset-bot bot commented Nov 10, 2023 •

edited

Loading

github-actions bot commented Nov 10, 2023 •

edited

Loading

phryneas commented Nov 10, 2023 •

edited

Loading