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

fix potential memory leak in Concast, add tests #11358

Merged
merged 19 commits into from
Nov 30, 2023

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Nov 10, 2023

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

@phryneas phryneas added this to the MemoryAnalysis milestone Nov 10, 2023
Copy link

changeset-bot bot commented Nov 10, 2023

🦋 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

Copy link
Contributor

github-actions bot commented Nov 10, 2023

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
Copy link
Member Author

phryneas commented Nov 10, 2023

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.

@phryneas phryneas marked this pull request as ready for review November 14, 2023 14:21
"@apollo/client": patch
---

Fixes a potential memory leak (that was not triggered by ApolloClient until now) in `Concast`.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

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

declare class WeakRef<T extends WeakKey> {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

declare class WeakRef<T extends WeakKey> {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

return {
pass,
message: () => {
if (pass) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

});

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

Choose a reason for hiding this comment

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

Minor nit: declaring variables on separate lines for readability

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

});

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

Choose a reason for hiding this comment

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

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

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

let observer!: Observer<number>;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@alessbell alessbell left a comment

Choose a reason for hiding this comment

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

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

@github-actions github-actions bot added the auto-cleanup 🤖 label Nov 29, 2023
@phryneas phryneas merged commit 7d939f8 into release-3.9 Nov 30, 2023
28 checks passed
@phryneas phryneas deleted the pr/concast-memoryLeaks branch November 30, 2023 14:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants