Skip to content

Commit

Permalink
Fix a memory leak related to ExecutionContext and Promises (#2215)
Browse files Browse the repository at this point in the history
  • Loading branch information
kamilkisiela committed Jun 21, 2022
1 parent d168926 commit 285987c
Show file tree
Hide file tree
Showing 13 changed files with 87 additions and 36 deletions.
5 changes: 5 additions & 0 deletions .changeset/wet-peaches-yell.md
@@ -0,0 +1,5 @@
---
'graphql-modules': patch
---

Fix a memory leak related to ExecutionContext and Promises
2 changes: 1 addition & 1 deletion .github/workflows/algolia-integrity.yml
Expand Up @@ -14,7 +14,7 @@ jobs:
- name: Use Node 18
uses: actions/setup-node@v3
with:
node-version: 17
node-version: 18
cache: 'yarn'
- name: Install Dependencies
run: yarn --ignore-engines
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/algolia-publish.yml
Expand Up @@ -16,7 +16,7 @@ jobs:
- name: Use Node
uses: actions/setup-node@v3
with:
node-version: 17
node-version: 18
cache: 'yarn'

- name: Install Dependencies
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/benchmark.yml
Expand Up @@ -8,7 +8,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
node_version: [12, 17]
node_version: [12, 14, 16, 18]
env:
CI: true

Expand All @@ -26,9 +26,9 @@ jobs:
name: Cache node_modules
with:
path: '**/node_modules'
key: ${{ runner.os }}-yarn-${{ matrix.node_version }}-15-${{ hashFiles('**/yarn.lock') }}
key: ${{ runner.os }}-yarn-${{ matrix.node_version }}-16-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn-${{ matrix.node_version }}-15-
${{ runner.os }}-yarn-${{ matrix.node_version }}-16-
${{ runner.os }}-yarn-${{ matrix.node_version }}-
${{ runner.os }}-yarn-
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/canary.yml
Expand Up @@ -18,7 +18,7 @@ jobs:
- name: Use Node
uses: actions/setup-node@v2
with:
node-version: 17
node-version: 18
- name: Configure Git Credentials
run: |
git config --global user.email "theguild-bot@users.noreply.github.com"
Expand All @@ -32,10 +32,10 @@ jobs:
name: Cache node_modules
with:
path: '**/node_modules'
key: ${{ runner.os }}-yarn-17-15-${{ hashFiles('**/yarn.lock') }}
key: ${{ runner.os }}-yarn-18-16-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn-17-15
${{ runner.os }}-yarn-17
${{ runner.os }}-yarn-18-16
${{ runner.os }}-yarn-18-
${{ runner.os }}-yarn-
- name: Install Dependencies using Yarn
run: yarn install --ignore-engines && git checkout yarn.lock
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/release.yml
Expand Up @@ -26,7 +26,7 @@ jobs:
- name: Use Node
uses: actions/setup-node@v2
with:
node-version: 17
node-version: 18
- name: Setup NPM credentials
run: echo "//registry.npmjs.org/:_authToken=$NODE_AUTH_TOKEN" >> ~/.npmrc
env:
Expand All @@ -35,10 +35,10 @@ jobs:
name: Cache node_modules
with:
path: '**/node_modules'
key: ${{ runner.os }}-yarn-17-15-${{ hashFiles('**/yarn.lock') }}
key: ${{ runner.os }}-yarn-18-16-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn-17-15-
${{ runner.os }}-yarn-17-
${{ runner.os }}-yarn-18-16-
${{ runner.os }}-yarn-18-
${{ runner.os }}-yarn-
- name: Install Dependencies using Yarn
run: yarn install --ignore-engines && git checkout yarn.lock
Expand Down
20 changes: 10 additions & 10 deletions .github/workflows/tests.yml
Expand Up @@ -16,14 +16,14 @@ jobs:
- name: Use Node
uses: actions/setup-node@master
with:
node-version: 17
node-version: 18
- name: Cache Yarn
uses: actions/cache@v2
with:
path: '**/node_modules'
key: ${{ runner.os }}-17-15-yarn-${{ hashFiles('yarn.lock') }}
key: ${{ runner.os }}-18-16-yarn-${{ hashFiles('yarn.lock') }}
restore-keys: |
${{ runner.os }}-17-15-yarn
${{ runner.os }}-18-16-yarn
- name: Install Dependencies using Yarn
run: yarn install --ignore-engines && git checkout yarn.lock
- name: Lint
Expand All @@ -43,14 +43,14 @@ jobs:
- name: Use Node
uses: actions/setup-node@master
with:
node-version: 17
node-version: 18
- name: Cache Yarn
uses: actions/cache@v2
with:
path: '**/node_modules'
key: ${{ runner.os }}-17-${{matrix.graphql_version}}-yarn-${{ hashFiles('yarn.lock') }}
key: ${{ runner.os }}-18-${{matrix.graphql_version}}-yarn-${{ hashFiles('yarn.lock') }}
restore-keys: |
${{ runner.os }}-17-${{matrix.graphql_version}}-yarn-
${{ runner.os }}-18-${{matrix.graphql_version}}-yarn-
- name: Use GraphQL v${{matrix.graphql_version}}
run: node ./scripts/match-graphql.js ${{matrix.graphql_version}}
- name: Install Dependencies using Yarn
Expand All @@ -63,7 +63,7 @@ jobs:
strategy:
matrix:
os: [ubuntu-latest] # remove windows to speed up the tests
node-version: [12, 17]
node-version: [12, 14, 16, 18]
graphql_version:
- 14
- 15
Expand Down Expand Up @@ -106,14 +106,14 @@ jobs:
- name: Use Node
uses: actions/setup-node@master
with:
node-version: 17
node-version: 18
- name: Cache Yarn
uses: actions/cache@v2
with:
path: '**/node_modules'
key: ${{ runner.os }}-17-15-yarn-${{ hashFiles('yarn.lock') }}
key: ${{ runner.os }}-18-16-yarn-${{ hashFiles('yarn.lock') }}
restore-keys: |
${{ runner.os }}-17-15-yarn-
${{ runner.os }}-18-16-yarn-
- name: Install Dependencies using Yarn
run: yarn install --ignore-engines && git checkout yarn.lock
- name: Build Packages
Expand Down
2 changes: 1 addition & 1 deletion jest.config.base.js
@@ -1,5 +1,5 @@
const { resolve } = require('path');
const { pathsToModuleNameMapper } = require('ts-jest/utils');
const { pathsToModuleNameMapper } = require('ts-jest');

const ROOT_DIR = __dirname;
const TSCONFIG_PATH = resolve(ROOT_DIR, 'tsconfig.json');
Expand Down
5 changes: 4 additions & 1 deletion packages/graphql-modules/src/application/context.ts
Expand Up @@ -91,7 +91,9 @@ export function createContextBuilder({
return getModuleContext(moduleId, context);
},
};
executionContext.create(executionContextPicker);
const destroyExecutionContext = executionContext.create(
executionContextPicker
);

// As the name of the Injector says, it's an Operation scoped Injector
// Application level
Expand Down Expand Up @@ -184,6 +186,7 @@ export function createContextBuilder({
injector._getObjByKeyId(keyId).onDestroy();
}
});
destroyExecutionContext();
contextCache = {};
}),
ɵinjector: operationAppInjector,
Expand Down
46 changes: 39 additions & 7 deletions packages/graphql-modules/src/application/execution-context.ts
Expand Up @@ -6,15 +6,20 @@ export interface ExecutionContextPicker {
}

const executionContextStore = new Map<number, ExecutionContextPicker>();
const executionContextDependencyStore = new Map<number, Set<number>>();

const executionContextHook = createHook({
init(asyncId, _, triggerAsyncId) {
// Store same context data for child async resources
if (executionContextStore.has(triggerAsyncId)) {
executionContextStore.set(
asyncId,
executionContextStore.get(triggerAsyncId)!
);
const ctx = executionContextStore.get(triggerAsyncId);
if (ctx) {
const dependencies =
executionContextDependencyStore.get(triggerAsyncId) ??
executionContextDependencyStore
.set(triggerAsyncId, new Set())
.get(triggerAsyncId)!;
dependencies.add(asyncId);
executionContextStore.set(asyncId, ctx);
}
},
destroy(asyncId) {
Expand All @@ -24,13 +29,32 @@ const executionContextHook = createHook({
},
});

function destroyContextAndItsChildren(id: number) {
if (executionContextStore.has(id)) {
executionContextStore.delete(id);
}

const deps = executionContextDependencyStore.get(id);

if (deps) {
for (const dep of deps) {
destroyContextAndItsChildren(dep);
}
executionContextDependencyStore.delete(id);
}
}

export const executionContext: {
create(picker: ExecutionContextPicker): void;
create(picker: ExecutionContextPicker): () => void;
getModuleContext: ExecutionContextPicker['getModuleContext'];
getApplicationContext: ExecutionContextPicker['getApplicationContext'];
} = {
create(picker) {
executionContextStore.set(executionAsyncId(), picker);
const id = executionAsyncId();
executionContextStore.set(id, picker);
return function destroyContext() {
destroyContextAndItsChildren(id);
};
},
getModuleContext(moduleId) {
const picker = executionContextStore.get(executionAsyncId())!;
Expand All @@ -49,3 +73,11 @@ export function enableExecutionContext() {
executionContextHook.enable();
}
}

export function getExecutionContextStore() {
return executionContextStore;
}

export function getExecutionContextDependencyStore() {
return executionContextDependencyStore;
}
2 changes: 1 addition & 1 deletion packages/graphql-modules/tests/di-providers.spec.ts
Expand Up @@ -226,7 +226,7 @@ test('general test', async () => {
expect(spies.logger).toHaveBeenCalledTimes(2);
});

test('useFactory with dependecies', async () => {
test('useFactory with dependencies', async () => {
const logSpy = jest.fn();

@Injectable({
Expand Down
11 changes: 11 additions & 0 deletions packages/graphql-modules/tests/execution-context.spec.ts
Expand Up @@ -9,9 +9,20 @@ import {
InjectionToken,
testkit,
} from '../src';
import {
getExecutionContextDependencyStore,
getExecutionContextStore,
} from '../src/application/execution-context';

const posts = ['Foo', 'Bar'];

afterEach(() => {
// There should be no execution context left after each test
expect(
getExecutionContextDependencyStore().size + getExecutionContextStore().size
).toBe(0);
});

test('ExecutionContext on module level provider', async () => {
const spies = {
posts: jest.fn(),
Expand Down
6 changes: 3 additions & 3 deletions packages/graphql-modules/tests/federation.spec.ts
Expand Up @@ -13,7 +13,7 @@ describe('federation', () => {
}

const {
buildFederatedSchema,
buildSubgraphSchema,
}: typeof import('@apollo/federation') = require('@apollo/federation');

test('allow __resolveReference', async () => {
Expand All @@ -40,7 +40,7 @@ describe('federation', () => {
createApplication({
modules: [mod],
schemaBuilder(input) {
return buildFederatedSchema({
return buildSubgraphSchema({
typeDefs: input.typeDefs,
resolvers: mergeResolvers(input.resolvers) as any,
});
Expand Down Expand Up @@ -73,7 +73,7 @@ describe('federation', () => {
createApplication({
modules: [mod],
schemaBuilder(input) {
return buildFederatedSchema({
return buildSubgraphSchema({
typeDefs: input.typeDefs,
resolvers: mergeResolvers(input.resolvers) as any,
});
Expand Down

1 comment on commit 285987c

@vercel
Copy link

@vercel vercel bot commented on 285987c Jun 21, 2022

Choose a reason for hiding this comment

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

Please sign in to comment.