From 9ac2133b9d45be92fa492290f7498cfb78dc3e5c Mon Sep 17 00:00:00 2001 From: enisdenjo Date: Sun, 22 Dec 2024 21:28:48 +0100 Subject: [PATCH 01/10] failing test --- .../tests/execution-context.spec.ts | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/packages/graphql-modules/tests/execution-context.spec.ts b/packages/graphql-modules/tests/execution-context.spec.ts index a41ac30bb8..45cd870b2a 100644 --- a/packages/graphql-modules/tests/execution-context.spec.ts +++ b/packages/graphql-modules/tests/execution-context.spec.ts @@ -798,3 +798,99 @@ test('accessing a singleton provider with execution context in another singleton }); } }); + +test('accessing a singleton provider context after another asynchronous execution', async () => { + @Injectable({ scope: Scope.Singleton }) + class IdentifierProvider { + @ExecutionContext() + private context: any; + getId() { + return this.context.identifier; + } + } + + const { promise: gettingBefore, resolve: gotBefore } = + Promise.withResolvers(); + + const { promise: waitForGettingAfter, resolve: getAfter } = + Promise.withResolvers(); + + const mod = createModule({ + id: 'mod', + providers: [IdentifierProvider], + typeDefs: gql` + type Query { + getAsyncIdentifiers: Identifiers! + } + + type Identifiers { + before: String! + after: String! + } + `, + resolvers: { + Query: { + async getAsyncIdentifiers( + _0: unknown, + _1: unknown, + context: GraphQLModules.Context + ) { + const before = context.injector.get(IdentifierProvider).getId(); + gotBefore(); + await waitForGettingAfter; + const after = context.injector.get(IdentifierProvider).getId(); + return { before, after }; + }, + }, + }, + }); + + const app = createApplication({ + modules: [mod], + }); + + const document = gql` + { + getAsyncIdentifiers { + before + after + } + } + `; + + const firstResult$ = testkit.execute(app, { + contextValue: { + identifier: 'first', + }, + document, + }); + + await gettingBefore; + + const secondResult$ = testkit.execute(app, { + contextValue: { + identifier: 'second', + }, + document, + }); + + getAfter(); + + await expect(firstResult$).resolves.toEqual({ + data: { + getAsyncIdentifiers: { + before: 'first', + after: 'first', + }, + }, + }); + + await expect(secondResult$).resolves.toEqual({ + data: { + getAsyncIdentifiers: { + before: 'second', + after: 'second', + }, + }, + }); +}); From 7d5150f892d86e75e90a82e1ef9279c30347502a Mon Sep 17 00:00:00 2001 From: enisdenjo Date: Sun, 22 Dec 2024 22:50:03 +0100 Subject: [PATCH 02/10] create deferred --- .../tests/execution-context.spec.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/packages/graphql-modules/tests/execution-context.spec.ts b/packages/graphql-modules/tests/execution-context.spec.ts index 45cd870b2a..781a334406 100644 --- a/packages/graphql-modules/tests/execution-context.spec.ts +++ b/packages/graphql-modules/tests/execution-context.spec.ts @@ -809,11 +809,9 @@ test('accessing a singleton provider context after another asynchronous executio } } - const { promise: gettingBefore, resolve: gotBefore } = - Promise.withResolvers(); + const { promise: gettingBefore, resolve: gotBefore } = createDeferred(); - const { promise: waitForGettingAfter, resolve: getAfter } = - Promise.withResolvers(); + const { promise: waitForGettingAfter, resolve: getAfter } = createDeferred(); const mod = createModule({ id: 'mod', @@ -894,3 +892,16 @@ test('accessing a singleton provider context after another asynchronous executio }, }); }); + +function createDeferred() { + let resolve!: (val: T) => void, reject!: (err: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + return { + promise, + resolve, + reject, + }; +} From c4ffcbc3c501666b84ae3f851c572fa81e67e7b0 Mon Sep 17 00:00:00 2001 From: enisdenjo Date: Sun, 22 Dec 2024 22:50:07 +0100 Subject: [PATCH 03/10] fix --- .../graphql-modules/src/application/apollo.ts | 46 +++++----- .../src/application/context.ts | 40 +++++++- .../src/application/execution.ts | 70 ++++++++------ .../src/application/subscription.ts | 91 ++++++++++--------- 4 files changed, 151 insertions(+), 96 deletions(-) diff --git a/packages/graphql-modules/src/application/apollo.ts b/packages/graphql-modules/src/application/apollo.ts index 6f51654dfa..9f61075feb 100644 --- a/packages/graphql-modules/src/application/apollo.ts +++ b/packages/graphql-modules/src/application/apollo.ts @@ -2,7 +2,7 @@ import { wrapSchema } from '@graphql-tools/wrap'; import { DocumentNode, execute, GraphQLSchema } from 'graphql'; import { uniqueId } from '../shared/utils'; import { InternalAppContext } from './application'; -import { ExecutionContextBuilder } from './context'; +import { ExecutionContextBuilder, ExecutionContextEnv } from './context'; import { Application } from './types'; const CONTEXT_ID = Symbol.for('context-id'); @@ -60,11 +60,12 @@ export function apolloSchemaCreator({ > = {}; const subscription = createSubscription(); - function getSession(ctx: any) { + function getSession( + ctx: any, + { context, ɵdestroy: destroy }: ExecutionContextEnv + ) { if (!ctx[CONTEXT_ID]) { ctx[CONTEXT_ID] = uniqueId((id) => !sessions[id]); - const { context, ɵdestroy: destroy } = contextBuilder(ctx); - sessions[ctx[CONTEXT_ID]] = { count: 0, session: { @@ -99,24 +100,27 @@ export function apolloSchemaCreator({ operationName: input.operationName, }); } - // Create an execution context - const { context, destroy } = getSession(input.context!); - // It's important to wrap the executeFn within a promise - // so we can easily control the end of execution (with finally) - return Promise.resolve() - .then( - () => - execute({ - schema, - document: input.document, - contextValue: context, - variableValues: input.variables as any, - rootValue: input.rootValue, - operationName: input.operationName, - }) as any - ) - .finally(destroy); + // Create an execution context and run within it + return contextBuilder(input.context!).runWithContext((env) => { + const { context, destroy } = getSession(input.context!, env); + + // It's important to wrap the executeFn within a promise + // so we can easily control the end of execution (with finally) + return Promise.resolve() + .then( + () => + execute({ + schema, + document: input.document, + contextValue: context, + variableValues: input.variables as any, + rootValue: input.rootValue, + operationName: input.operationName, + }) as any + ) + .finally(destroy); + }); }, }); }; diff --git a/packages/graphql-modules/src/application/context.ts b/packages/graphql-modules/src/application/context.ts index 69ab7c258b..13ccab21c8 100644 --- a/packages/graphql-modules/src/application/context.ts +++ b/packages/graphql-modules/src/application/context.ts @@ -1,3 +1,4 @@ +import { AsyncLocalStorage } from 'async_hooks'; import { Injector, ReflectiveInjector } from '../di'; import { ResolvedProvider } from '../di/resolution'; import { ID } from '../shared/types'; @@ -6,11 +7,22 @@ import type { InternalAppContext, ModulesMap } from './application'; import { attachGlobalProvidersMap } from './di'; import { CONTEXT } from './tokens'; +const alc = new AsyncLocalStorage<{ + getApplicationContext(): GraphQLModules.AppContext; + getModuleContext(moduleId: string): GraphQLModules.ModuleContext; +}>(); + export type ExecutionContextBuilder< TContext extends { [key: string]: any; } = {}, -> = (context: TContext) => { +> = (context: TContext) => ExecutionContextEnv & { + runWithContext( + cb: (env: ExecutionContextEnv) => TReturn + ): TReturn; +}; + +export type ExecutionContextEnv = { context: InternalAppContext; ɵdestroy(): void; ɵinjector: Injector; @@ -67,12 +79,15 @@ export function createContextBuilder({ }); appInjector.setExecutionContextGetter(function executionContextGetter() { - return appContext; + return alc.getStore()?.getApplicationContext() || appContext; } as any); function createModuleExecutionContextGetter(moduleId: string) { return function moduleExecutionContextGetter() { - return getModuleContext(moduleId, context); + return ( + alc.getStore()?.getModuleContext(moduleId) || + getModuleContext(moduleId, context) + ); }; } @@ -164,7 +179,7 @@ export function createContextBuilder({ }, }); - return { + const env: ExecutionContextEnv = { ɵdestroy: once(() => { providersToDestroy.forEach(([injector, keyId]) => { // If provider was instantiated @@ -178,6 +193,23 @@ export function createContextBuilder({ ɵinjector: operationAppInjector, context: sharedContext, }; + + return { + ...env, + runWithContext(cb) { + return alc.run( + { + getApplicationContext() { + return appContext; + }, + getModuleContext(moduleId) { + return getModuleContext(moduleId, context); + }, + }, + () => cb(env) + ); + }, + }; }; return contextBuilder; diff --git a/packages/graphql-modules/src/application/execution.ts b/packages/graphql-modules/src/application/execution.ts index 261a3358a2..5061eefff6 100644 --- a/packages/graphql-modules/src/application/execution.ts +++ b/packages/graphql-modules/src/application/execution.ts @@ -10,6 +10,7 @@ import { Application } from './types'; import { ExecutionContextBuilder } from './context'; import { Maybe } from '../shared/types'; import { isNotSchema } from '../shared/utils'; +import { InternalAppContext } from './application'; export function executionCreator({ contextBuilder, @@ -30,38 +31,47 @@ export function executionCreator({ fieldResolver?: Maybe>, typeResolver?: Maybe> ) => { - // Create an execution context - const { context, ɵdestroy: destroy } = - options?.controller ?? - contextBuilder( - isNotSchema(argsOrSchema) - ? argsOrSchema.contextValue - : contextValue - ); + function perform({ + context, + ɵdestroy: destroy, + }: { + context: InternalAppContext; + ɵdestroy: () => void; + }) { + const executionArgs: ExecutionArgs = isNotSchema( + argsOrSchema + ) + ? { + ...argsOrSchema, + contextValue: context, + } + : { + schema: argsOrSchema, + document: document!, + rootValue, + contextValue: context, + variableValues, + operationName, + fieldResolver, + typeResolver, + }; - const executionArgs: ExecutionArgs = isNotSchema( - argsOrSchema - ) - ? { - ...argsOrSchema, - contextValue: context, - } - : { - schema: argsOrSchema, - document: document!, - rootValue, - contextValue: context, - variableValues, - operationName, - fieldResolver, - typeResolver, - }; + // It's important to wrap the executeFn within a promise + // so we can easily control the end of execution (with finally) + return Promise.resolve() + .then(() => executeFn(executionArgs)) + .finally(destroy); + } - // It's important to wrap the executeFn within a promise - // so we can easily control the end of execution (with finally) - return Promise.resolve() - .then(() => executeFn(executionArgs)) - .finally(destroy); + if (options?.controller) { + return perform(options.controller); + } + + return contextBuilder( + isNotSchema(argsOrSchema) + ? argsOrSchema.contextValue + : contextValue + ).runWithContext(perform); }; }; diff --git a/packages/graphql-modules/src/application/subscription.ts b/packages/graphql-modules/src/application/subscription.ts index 530ef44e3f..0770306851 100644 --- a/packages/graphql-modules/src/application/subscription.ts +++ b/packages/graphql-modules/src/application/subscription.ts @@ -13,6 +13,7 @@ import { } from '../shared/utils'; import { ExecutionContextBuilder } from './context'; import { Application } from './types'; +import { InternalAppContext } from './application'; export function subscriptionCreator({ contextBuilder, @@ -33,51 +34,59 @@ export function subscriptionCreator({ fieldResolver?: Maybe>, subscribeFieldResolver?: Maybe> ) => { - // Create an subscription context - const { context, ɵdestroy: destroy } = - options?.controller ?? - contextBuilder( + function perform({ + context, + ɵdestroy: destroy, + }: { + context: InternalAppContext; + ɵdestroy: () => void; + }) { + const subscriptionArgs: SubscriptionArgs = isNotSchema(argsOrSchema) - ? argsOrSchema.contextValue - : contextValue - ); + ? { + ...argsOrSchema, + contextValue: context, + } + : { + schema: argsOrSchema, + document: document!, + rootValue, + contextValue: context, + variableValues, + operationName, + fieldResolver, + subscribeFieldResolver, + }; - const subscriptionArgs: SubscriptionArgs = isNotSchema( - argsOrSchema - ) - ? { - ...argsOrSchema, - contextValue: context, - } - : { - schema: argsOrSchema, - document: document!, - rootValue, - contextValue: context, - variableValues, - operationName, - fieldResolver, - subscribeFieldResolver, - }; + let isIterable = false; - let isIterable = false; + // It's important to wrap the subscribeFn within a promise + // so we can easily control the end of subscription (with finally) + return Promise.resolve() + .then(() => subscribeFn(subscriptionArgs)) + .then((sub) => { + if (isAsyncIterable(sub)) { + isIterable = true; + return tapAsyncIterator(sub, destroy); + } + return sub; + }) + .finally(() => { + if (!isIterable) { + destroy(); + } + }); + } - // It's important to wrap the subscribeFn within a promise - // so we can easily control the end of subscription (with finally) - return Promise.resolve() - .then(() => subscribeFn(subscriptionArgs)) - .then((sub) => { - if (isAsyncIterable(sub)) { - isIterable = true; - return tapAsyncIterator(sub, destroy); - } - return sub; - }) - .finally(() => { - if (!isIterable) { - destroy(); - } - }); + if (options?.controller) { + return perform(options.controller); + } + + return contextBuilder( + isNotSchema(argsOrSchema) + ? argsOrSchema.contextValue + : contextValue + ).runWithContext(perform); }; }; From 674927c3a29a8c57bbdad7eb31576cb88178f815 Mon Sep 17 00:00:00 2001 From: enisdenjo Date: Mon, 23 Dec 2024 15:43:31 +0100 Subject: [PATCH 04/10] changeset --- .changeset/shiny-turkeys-dream.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/shiny-turkeys-dream.md diff --git a/.changeset/shiny-turkeys-dream.md b/.changeset/shiny-turkeys-dream.md new file mode 100644 index 0000000000..65823bcbe2 --- /dev/null +++ b/.changeset/shiny-turkeys-dream.md @@ -0,0 +1,5 @@ +--- +'graphql-modules': patch +--- + +Bind context to async execution avoiding race-conditions From a8f6c9260884be8144dbc44aa9db54009b5f041d Mon Sep 17 00:00:00 2001 From: enisdenjo Date: Mon, 23 Dec 2024 16:10:18 +0100 Subject: [PATCH 05/10] cb and args --- packages/graphql-modules/src/application/context.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/graphql-modules/src/application/context.ts b/packages/graphql-modules/src/application/context.ts index 13ccab21c8..41c206ba27 100644 --- a/packages/graphql-modules/src/application/context.ts +++ b/packages/graphql-modules/src/application/context.ts @@ -206,7 +206,8 @@ export function createContextBuilder({ return getModuleContext(moduleId, context); }, }, - () => cb(env) + cb, + env ); }, }; From 8750a83885a9c3564903d48a639541391c8e9a6d Mon Sep 17 00:00:00 2001 From: enisdenjo Date: Tue, 24 Dec 2024 13:51:18 +0100 Subject: [PATCH 06/10] async-context --- .../src/application/async-context.ts | 32 +++++++++++++++++++ .../src/application/context.ts | 13 +++----- 2 files changed, 36 insertions(+), 9 deletions(-) create mode 100644 packages/graphql-modules/src/application/async-context.ts diff --git a/packages/graphql-modules/src/application/async-context.ts b/packages/graphql-modules/src/application/async-context.ts new file mode 100644 index 0000000000..23427a3e6f --- /dev/null +++ b/packages/graphql-modules/src/application/async-context.ts @@ -0,0 +1,32 @@ +import type { AsyncLocalStorage } from 'async_hooks'; +import module from 'module'; + +export interface AsyncContext { + getApplicationContext(): GraphQLModules.AppContext; + getModuleContext(moduleId: string): GraphQLModules.ModuleContext; +} + +let alc: AsyncLocalStorage | undefined; +if (typeof process !== 'undefined') { + // probably nodejs runtime + const require = module.createRequire( + 'file:///' /** path is not relevant since we're only loading a builtin */ + ); + const hooks = require('async_hooks') as typeof import('async_hooks'); + alc = new hooks.AsyncLocalStorage(); +} + +export function getAsyncContext() { + return alc?.getStore(); +} + +export function runWithAsyncContext( + asyncContext: AsyncContext, + callback: (...args: TArgs) => R, + ...args: TArgs +): R { + if (!alc) { + return callback(...args); + } + return alc.run(asyncContext, callback, ...args); +} diff --git a/packages/graphql-modules/src/application/context.ts b/packages/graphql-modules/src/application/context.ts index 41c206ba27..734153ff9d 100644 --- a/packages/graphql-modules/src/application/context.ts +++ b/packages/graphql-modules/src/application/context.ts @@ -1,17 +1,12 @@ -import { AsyncLocalStorage } from 'async_hooks'; import { Injector, ReflectiveInjector } from '../di'; import { ResolvedProvider } from '../di/resolution'; import { ID } from '../shared/types'; import { once, merge } from '../shared/utils'; import type { InternalAppContext, ModulesMap } from './application'; +import { getAsyncContext, runWithAsyncContext } from './async-context'; import { attachGlobalProvidersMap } from './di'; import { CONTEXT } from './tokens'; -const alc = new AsyncLocalStorage<{ - getApplicationContext(): GraphQLModules.AppContext; - getModuleContext(moduleId: string): GraphQLModules.ModuleContext; -}>(); - export type ExecutionContextBuilder< TContext extends { [key: string]: any; @@ -79,13 +74,13 @@ export function createContextBuilder({ }); appInjector.setExecutionContextGetter(function executionContextGetter() { - return alc.getStore()?.getApplicationContext() || appContext; + return getAsyncContext()?.getApplicationContext() || appContext; } as any); function createModuleExecutionContextGetter(moduleId: string) { return function moduleExecutionContextGetter() { return ( - alc.getStore()?.getModuleContext(moduleId) || + getAsyncContext()?.getModuleContext(moduleId) || getModuleContext(moduleId, context) ); }; @@ -197,7 +192,7 @@ export function createContextBuilder({ return { ...env, runWithContext(cb) { - return alc.run( + return runWithAsyncContext( { getApplicationContext() { return appContext; From 55ecc95f434893991314f7e466638fe302dc7739 Mon Sep 17 00:00:00 2001 From: enisdenjo Date: Tue, 24 Dec 2024 14:07:29 +0100 Subject: [PATCH 07/10] during* --- packages/graphql-modules/tests/execution-context.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/graphql-modules/tests/execution-context.spec.ts b/packages/graphql-modules/tests/execution-context.spec.ts index 781a334406..f395f398f1 100644 --- a/packages/graphql-modules/tests/execution-context.spec.ts +++ b/packages/graphql-modules/tests/execution-context.spec.ts @@ -799,7 +799,7 @@ test('accessing a singleton provider with execution context in another singleton } }); -test('accessing a singleton provider context after another asynchronous execution', async () => { +test('accessing a singleton provider context during another asynchronous execution', async () => { @Injectable({ scope: Scope.Singleton }) class IdentifierProvider { @ExecutionContext() From 661de3ec35f3be380b3880b04c38bb1bd8b17e15 Mon Sep 17 00:00:00 2001 From: Denis Badurina Date: Tue, 21 Jan 2025 16:56:50 +0100 Subject: [PATCH 08/10] use require directly (?) --- packages/graphql-modules/src/application/async-context.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/graphql-modules/src/application/async-context.ts b/packages/graphql-modules/src/application/async-context.ts index 23427a3e6f..2fb59ac04f 100644 --- a/packages/graphql-modules/src/application/async-context.ts +++ b/packages/graphql-modules/src/application/async-context.ts @@ -1,5 +1,4 @@ import type { AsyncLocalStorage } from 'async_hooks'; -import module from 'module'; export interface AsyncContext { getApplicationContext(): GraphQLModules.AppContext; @@ -8,10 +7,6 @@ export interface AsyncContext { let alc: AsyncLocalStorage | undefined; if (typeof process !== 'undefined') { - // probably nodejs runtime - const require = module.createRequire( - 'file:///' /** path is not relevant since we're only loading a builtin */ - ); const hooks = require('async_hooks') as typeof import('async_hooks'); alc = new hooks.AsyncLocalStorage(); } From d36fd7cda373ad6eab46df91f1c70d186f6e2843 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 21 Jan 2025 15:57:12 +0000 Subject: [PATCH 09/10] chore(dependencies): updated changesets for modified dependencies --- .changeset/graphql-modules-2521-dependencies.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/graphql-modules-2521-dependencies.md diff --git a/.changeset/graphql-modules-2521-dependencies.md b/.changeset/graphql-modules-2521-dependencies.md new file mode 100644 index 0000000000..89f9cf6f48 --- /dev/null +++ b/.changeset/graphql-modules-2521-dependencies.md @@ -0,0 +1,5 @@ +--- +'graphql-modules': patch +--- +dependencies updates: + - Updated dependency [`ramda@^0.30.0` ↗︎](https://www.npmjs.com/package/ramda/v/0.30.0) (from `^0.29.0`, in `dependencies`) From 0491f4c48e11d9b869a28a3d5dc1f2c339cfdb5c Mon Sep 17 00:00:00 2001 From: Denis Badurina Date: Tue, 21 Jan 2025 17:03:18 +0100 Subject: [PATCH 10/10] Revert "use require directly (?)" This reverts commit 661de3ec35f3be380b3880b04c38bb1bd8b17e15. --- packages/graphql-modules/src/application/async-context.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/graphql-modules/src/application/async-context.ts b/packages/graphql-modules/src/application/async-context.ts index 2fb59ac04f..23427a3e6f 100644 --- a/packages/graphql-modules/src/application/async-context.ts +++ b/packages/graphql-modules/src/application/async-context.ts @@ -1,4 +1,5 @@ import type { AsyncLocalStorage } from 'async_hooks'; +import module from 'module'; export interface AsyncContext { getApplicationContext(): GraphQLModules.AppContext; @@ -7,6 +8,10 @@ export interface AsyncContext { let alc: AsyncLocalStorage | undefined; if (typeof process !== 'undefined') { + // probably nodejs runtime + const require = module.createRequire( + 'file:///' /** path is not relevant since we're only loading a builtin */ + ); const hooks = require('async_hooks') as typeof import('async_hooks'); alc = new hooks.AsyncLocalStorage(); }