diff --git a/integration/injector/e2e/many-global-modules.spec.ts b/integration/injector/e2e/many-global-modules.spec.ts new file mode 100644 index 000000000..887d60d4e --- /dev/null +++ b/integration/injector/e2e/many-global-modules.spec.ts @@ -0,0 +1,130 @@ +import { Test } from '@nestjs/testing'; +import { expect } from 'chai'; +import * as sinon from 'sinon'; +import { Global, Inject, Injectable, Module, Scope } from '@nestjs/common'; + +@Global() +@Module({}) +export class GlobalModule1 {} + +@Global() +@Module({}) +export class GlobalModule2 {} + +@Global() +@Module({}) +export class GlobalModule3 {} + +@Global() +@Module({}) +export class GlobalModule4 {} + +@Global() +@Module({}) +export class GlobalModule5 {} + +@Global() +@Module({}) +export class GlobalModule6 {} + +@Global() +@Module({}) +export class GlobalModule7 {} + +@Global() +@Module({}) +export class GlobalModule8 {} + +@Global() +@Module({}) +export class GlobalModule9 {} + +@Global() +@Module({}) +export class GlobalModule10 {} + +@Injectable() +class TransientProvider {} + +@Injectable() +class RequestProvider {} + +@Injectable() +export class Dependant { + constructor( + private readonly transientProvider: TransientProvider, + + @Inject(RequestProvider) + private readonly requestProvider: RequestProvider, + ) {} + + public checkDependencies() { + expect(this.transientProvider).to.be.instanceOf(TransientProvider); + expect(this.requestProvider).to.be.instanceOf(RequestProvider); + } +} + +@Global() +@Module({ + providers: [ + { + provide: TransientProvider, + scope: Scope.TRANSIENT, + useClass: TransientProvider, + }, + { + provide: Dependant, + scope: Scope.DEFAULT, + useClass: Dependant, + }, + ], +}) +export class GlobalModuleWithTransientProviderAndDependant {} + +@Global() +@Module({ + providers: [ + { + provide: RequestProvider, + scope: Scope.REQUEST, + useFactory: () => { + return new RequestProvider(); + }, + }, + ], + exports: [RequestProvider], +}) +export class GlobalModuleWithRequestProvider {} + +@Module({ + imports: [ + GlobalModule1, + GlobalModule2, + GlobalModule3, + GlobalModule4, + GlobalModule5, + GlobalModule6, + GlobalModule7, + GlobalModule8, + GlobalModule9, + GlobalModule10, + GlobalModuleWithTransientProviderAndDependant, + GlobalModuleWithRequestProvider, + ], +}) +export class AppModule {} + +describe('Many global modules', () => { + it('should inject request-scoped useFactory provider and transient-scoped useClass provider from different modules', async () => { + const moduleBuilder = Test.createTestingModule({ + imports: [AppModule], + }); + const moduleRef = await moduleBuilder.compile(); + + const dependant = await moduleRef.resolve(Dependant); + const checkDependenciesSpy = sinon.spy(dependant, 'checkDependencies'); + dependant.checkDependencies(); + + expect(checkDependenciesSpy.called).to.be.true; + }); +}); diff --git a/integration/inspector/e2e/fixtures/post-init-graph.json b/integration/inspector/e2e/fixtures/post-init-graph.json index be476d57e..14739d76a 100644 --- a/integration/inspector/e2e/fixtures/post-init-graph.json +++ b/integration/inspector/e2e/fixtures/post-init-graph.json @@ -2197,22 +2197,6 @@ }, "id": "1976848738" }, - "-2105726668": { - "source": "-1803759743", - "target": "1010833816", - "metadata": { - "type": "class-to-class", - "sourceModuleName": "PropertiesModule", - "sourceClassName": "PropertiesService", - "targetClassName": "token", - "sourceClassToken": "PropertiesService", - "targetClassToken": "token", - "targetModuleName": "PropertiesModule", - "keyOrIndex": "token", - "injectionType": "property" - }, - "id": "-2105726668" - }, "-21463590": { "source": "-1378706112", "target": "1004276345", @@ -2229,6 +2213,22 @@ }, "id": "-21463590" }, + "-2105726668": { + "source": "-1803759743", + "target": "1010833816", + "metadata": { + "type": "class-to-class", + "sourceModuleName": "PropertiesModule", + "sourceClassName": "PropertiesService", + "targetClassName": "token", + "sourceClassToken": "PropertiesService", + "targetClassToken": "token", + "targetModuleName": "PropertiesModule", + "keyOrIndex": "token", + "injectionType": "property" + }, + "id": "-2105726668" + }, "-1657371464": { "source": "-1673986099", "target": "1919157847", diff --git a/packages/core/helpers/barrier.ts b/packages/core/helpers/barrier.ts new file mode 100644 index 000000000..c1c77d334 --- /dev/null +++ b/packages/core/helpers/barrier.ts @@ -0,0 +1,51 @@ +/** + * A simple barrier to synchronize flow of multiple async operations. + */ +export class Barrier { + private currentCount: number; + private targetCount: number; + private promise: Promise; + private resolve: () => void; + + constructor(targetCount: number) { + this.currentCount = 0; + this.targetCount = targetCount; + + this.promise = new Promise(resolve => { + this.resolve = resolve; + }); + } + + /** + * Signal that a participant has reached the barrier. + * + * The barrier will be resolved once `targetCount` participants have reached it. + */ + public signal(): void { + this.currentCount += 1; + if (this.currentCount === this.targetCount) { + this.resolve(); + } + } + + /** + * Wait for the barrier to be resolved. + * + * @returns A promise that resolves when the barrier is resolved. + */ + public async wait(): Promise { + return this.promise; + } + + /** + * Signal that a participant has reached the barrier and wait for the barrier to be resolved. + * + * The barrier will be resolved once `targetCount` participants have reached it. + * + * @returns A promise that resolves when the barrier is resolved. + */ + public async signalAndWait(): Promise { + this.signal(); + return this.wait(); + } +} diff --git a/packages/core/injector/injector.ts b/packages/core/injector/injector.ts index 4143ef784..4cf37e2af 100644 --- a/packages/core/injector/injector.ts +++ b/packages/core/injector/injector.ts @@ -42,6 +42,7 @@ import { } from './instance-wrapper'; import { Module } from './module'; import { SettlementSignal } from './settlement-signal'; +import { Barrier } from '../helpers/barrier'; /** * The type of an injectable dependency @@ -313,10 +314,16 @@ export class Injector { ? this.getFactoryProviderDependencies(wrapper) : this.getClassDependencies(wrapper); + const paramBarrier = new Barrier(dependencies.length); let isResolved = true; const resolveParam = async (param: unknown, index: number) => { try { if (this.isInquirer(param, parentInquirer)) { + /* + * Signal the barrier to make sure other dependencies do not get stuck waiting forever. + */ + paramBarrier.signal(); + return parentInquirer && parentInquirer.instance; } if (inquirer?.isTransient && parentInquirer) { @@ -332,15 +339,36 @@ export class Injector { inquirer, index, ); - const instanceHost = paramWrapper.getInstanceByContextId( - this.getContextId(contextId, paramWrapper), + + /* + * Ensure that all instance wrappers are resolved at this point before we continue. + * Otherwise the staticity of `wrapper`'s dependency tree may be evaluated incorrectly + * and result in undefined / null injection. + */ + await paramBarrier.signalAndWait(); + + const paramWrapperWithInstance = await this.resolveComponentHost( + moduleRef, + paramWrapper, + contextId, + inquirer, + ); + const instanceHost = paramWrapperWithInstance.getInstanceByContextId( + this.getContextId(contextId, paramWrapperWithInstance), inquirerId, ); - if (!instanceHost.isResolved && !paramWrapper.forwardRef) { + if (!instanceHost.isResolved && !paramWrapperWithInstance.forwardRef) { isResolved = false; } return instanceHost?.instance; } catch (err) { + /* + * Signal the barrier to make sure other dependencies do not get stuck waiting forever. We + * do not care if this occurs after `Barrier.signalAndWait()` is called in the `try` block + * because the barrier will always have been resolved by then. + */ + paramBarrier.signal(); + const isOptional = optionalDependenciesIds.includes(index); if (!isOptional) { throw err; @@ -440,7 +468,7 @@ export class Injector { ); } const token = this.resolveParamToken(wrapper, param); - return this.resolveComponentInstance( + return this.resolveComponentWrapper( moduleRef, token, dependencyContext, @@ -462,7 +490,7 @@ export class Injector { return param; } - public async resolveComponentInstance( + public async resolveComponentWrapper( moduleRef: Module, token: InjectionToken, dependencyContext: InjectorDependencyContext, @@ -474,7 +502,7 @@ export class Injector { this.printResolvingDependenciesLog(token, inquirer); this.printLookingForProviderLog(token, moduleRef); const providers = moduleRef.providers; - const instanceWrapper = await this.lookupComponent( + return this.lookupComponent( providers, moduleRef, { ...dependencyContext, name: token }, @@ -483,13 +511,6 @@ export class Injector { inquirer, keyOrIndex, ); - - return this.resolveComponentHost( - moduleRef, - instanceWrapper, - contextId, - inquirer, - ); } public async resolveComponentHost( @@ -689,6 +710,7 @@ export class Injector { return this.loadPropertiesMetadata(metadata, contextId, inquirer); } const properties = this.reflectProperties(wrapper.metatype as Type); + const propertyBarrier = new Barrier(properties.length); const instances = await Promise.all( properties.map(async (item: PropertyDependency) => { try { @@ -697,6 +719,11 @@ export class Injector { name: item.name as Function | string | symbol, }; if (this.isInquirer(item.name, parentInquirer)) { + /* + * Signal the barrier to make sure other dependencies do not get stuck waiting forever. + */ + propertyBarrier.signal(); + return parentInquirer && parentInquirer.instance; } const paramWrapper = await this.resolveSingleParam( @@ -708,16 +735,37 @@ export class Injector { inquirer, item.key, ); - if (!paramWrapper) { + + /* + * Ensure that all instance wrappers are resolved at this point before we continue. + * Otherwise the staticity of `wrapper`'s dependency tree may be evaluated incorrectly + * and result in undefined / null injection. + */ + await propertyBarrier.signalAndWait(); + + const paramWrapperWithInstance = await this.resolveComponentHost( + moduleRef, + paramWrapper, + contextId, + inquirer, + ); + if (!paramWrapperWithInstance) { return undefined; } const inquirerId = this.getInquirerId(inquirer); - const instanceHost = paramWrapper.getInstanceByContextId( - this.getContextId(contextId, paramWrapper), + const instanceHost = paramWrapperWithInstance.getInstanceByContextId( + this.getContextId(contextId, paramWrapperWithInstance), inquirerId, ); return instanceHost.instance; } catch (err) { + /* + * Signal the barrier to make sure other dependencies do not get stuck waiting forever. We + * do not care if this occurs after `Barrier.signalAndWait()` is called in the `try` block + * because the barrier will always have been resolved by then. + */ + propertyBarrier.signal(); + if (!item.isOptional) { throw err; } diff --git a/packages/core/test/helpers/barrier.spec.ts b/packages/core/test/helpers/barrier.spec.ts new file mode 100644 index 000000000..e7eefe2a2 --- /dev/null +++ b/packages/core/test/helpers/barrier.spec.ts @@ -0,0 +1,93 @@ +import { expect } from 'chai'; +import { Barrier } from '../../../core/helpers/barrier'; +import * as sinon from 'sinon'; +import * as chai from 'chai'; +import * as chaiAsPromised from 'chai-as-promised'; +import { setTimeout } from 'timers/promises'; +chai.use(chaiAsPromised); + +describe('Barrier', () => { + const targetCount = 3; + let barrier: Barrier; + let barrierResolveSpy: sinon.SinonSpy; + + beforeEach(() => { + barrier = new Barrier(targetCount); + barrierResolveSpy = sinon.spy(barrier, 'resolve'); + }); + + afterEach(() => { + // resolve any promises that may still be waiting in the background + (barrier).resolve(); + }); + + describe('signal', () => { + it('should resolve the barrier when target count is reached', async () => { + for (let i = 0; i < targetCount; i++) { + barrier.signal(); + } + + expect(barrierResolveSpy.called).to.be.true; + }); + + it('should not resolve the barrier when target count is not reached', async () => { + for (let i = 0; i < targetCount - 1; i++) { + barrier.signal(); + } + + expect(barrierResolveSpy.called).to.be.false; + expect((barrier).currentCount).to.be.equal(targetCount - 1); + }); + }); + + describe('wait', () => { + it('should resolve when target count is reached', async () => { + const waitPromise = barrier.wait(); + + for (let i = 0; i < targetCount; i++) { + barrier.signal(); + } + + expect(waitPromise).to.be.fulfilled; + }); + + it('should not resolve when target count is not reached', async () => { + const waitPromise = barrier.wait(); + + for (let i = 0; i < targetCount - 1; i++) { + barrier.signal(); + } + + expect(waitPromise).not.to.be.fulfilled; + }); + }); + + describe('signalAndWait', () => { + it('should resolve when target count is reached', async () => { + const promise = Promise.all( + Array.from({ length: targetCount }, () => barrier.signalAndWait()), + ); + + // wait for the promise to be resolved + await promise; + + expect(promise).to.be.fulfilled; + expect(barrierResolveSpy.called).to.be.true; + }); + + it('should not resolve when target count is not reached', async () => { + const promise = Promise.all( + Array.from({ length: targetCount - 1 }, () => barrier.signalAndWait()), + ); + + /* + * Give the promise some time to work. We cannot await the promise because the test case would + * get stuck. + */ + await setTimeout(5); + + expect(promise).not.to.be.fulfilled; + expect(barrierResolveSpy.called).to.be.false; + }); + }); +}); diff --git a/packages/core/test/injector/injector.spec.ts b/packages/core/test/injector/injector.spec.ts index 3b28e14fa..0aaab3f3c 100644 --- a/packages/core/test/injector/injector.spec.ts +++ b/packages/core/test/injector/injector.spec.ts @@ -545,7 +545,7 @@ describe('Injector', () => { }); }); - describe('resolveComponentInstance', () => { + describe('resolveComponentHost', () => { let module: any; beforeEach(() => { module = { @@ -560,16 +560,8 @@ describe('Injector', () => { const loadStub = sinon .stub(injector, 'loadProvider') .callsFake(() => null!); - sinon - .stub(injector, 'lookupComponent') - .returns(Promise.resolve(wrapper)); - await injector.resolveComponentInstance( - module, - '', - { index: 0, dependencies: [] }, - wrapper, - ); + await injector.resolveComponentHost(module, wrapper); expect(loadStub.called).to.be.true; }); it('should not call loadProvider (isResolved)', async () => { @@ -578,16 +570,7 @@ describe('Injector', () => { .stub(injector, 'loadProvider') .callsFake(() => null!); - sinon - .stub(injector, 'lookupComponent') - .returns(Promise.resolve(wrapper)); - - await injector.resolveComponentInstance( - module, - '', - { index: 0, dependencies: [] }, - wrapper, - ); + await injector.resolveComponentHost(module, wrapper); expect(loadStub.called).to.be.false; }); it('should not call loadProvider (forwardRef)', async () => { @@ -599,16 +582,7 @@ describe('Injector', () => { .stub(injector, 'loadProvider') .callsFake(() => null!); - sinon - .stub(injector, 'lookupComponent') - .returns(Promise.resolve(wrapper)); - - await injector.resolveComponentInstance( - module, - '', - { index: 0, dependencies: [] }, - wrapper, - ); + await injector.resolveComponentHost(module, wrapper); expect(loadStub.called).to.be.false; }); }); @@ -624,16 +598,8 @@ describe('Injector', () => { async: true, instance, }); - sinon - .stub(injector, 'lookupComponent') - .returns(Promise.resolve(wrapper)); - const result = await injector.resolveComponentInstance( - module, - '', - { index: 0, dependencies: [] }, - wrapper, - ); + const result = await injector.resolveComponentHost(module, wrapper); expect(result.instance).to.be.true; }); }); diff --git a/packages/testing/testing-injector.ts b/packages/testing/testing-injector.ts index 603544801..c5a783196 100644 --- a/packages/testing/testing-injector.ts +++ b/packages/testing/testing-injector.ts @@ -23,7 +23,7 @@ export class TestingInjector extends Injector { this.container = container; } - public async resolveComponentInstance( + public async resolveComponentWrapper( moduleRef: Module, name: any, dependencyContext: InjectorDependencyContext, @@ -33,7 +33,7 @@ export class TestingInjector extends Injector { keyOrIndex?: string | number, ): Promise { try { - const existingProviderWrapper = await super.resolveComponentInstance( + const existingProviderWrapper = await super.resolveComponentWrapper( moduleRef, name, dependencyContext, @@ -44,39 +44,72 @@ export class TestingInjector extends Injector { ); return existingProviderWrapper; } catch (err) { - if (this.mocker) { - const mockedInstance = this.mocker(name); - if (!mockedInstance) { - throw err; - } - const newWrapper = new InstanceWrapper({ - name, - isAlias: false, - scope: wrapper.scope, - instance: mockedInstance, - isResolved: true, - host: moduleRef, - metatype: wrapper.metatype, - }); - const internalCoreModule = this.container.getInternalCoreModuleRef(); - if (!internalCoreModule) { - throw new Error( - 'Expected to have internal core module reference at this point.', - ); - } - - internalCoreModule.addCustomProvider( - { - provide: name, - useValue: mockedInstance, - }, - internalCoreModule.providers, - ); - internalCoreModule.addExportedProviderOrModule(name); - return newWrapper; - } else { - throw err; - } + return this.mockWrapper(err, moduleRef, name, wrapper); } } + + public async resolveComponentHost( + moduleRef: Module, + instanceWrapper: InstanceWrapper, + contextId = STATIC_CONTEXT, + inquirer?: InstanceWrapper, + ): Promise { + try { + const existingProviderWrapper = await super.resolveComponentHost( + moduleRef, + instanceWrapper, + contextId, + inquirer, + ); + return existingProviderWrapper; + } catch (err) { + return this.mockWrapper( + err, + moduleRef, + instanceWrapper.name, + instanceWrapper, + ); + } + } + + private async mockWrapper( + err: Error, + moduleRef: Module, + name: any, + wrapper: InstanceWrapper, + ): Promise { + if (!this.mocker) { + throw err; + } + + const mockedInstance = this.mocker(name); + if (!mockedInstance) { + throw err; + } + const newWrapper = new InstanceWrapper({ + name, + isAlias: false, + scope: wrapper.scope, + instance: mockedInstance, + isResolved: true, + host: moduleRef, + metatype: wrapper.metatype, + }); + const internalCoreModule = this.container.getInternalCoreModuleRef(); + if (!internalCoreModule) { + throw new Error( + 'Expected to have internal core module reference at this point.', + ); + } + + internalCoreModule.addCustomProvider( + { + provide: name, + useValue: mockedInstance, + }, + internalCoreModule.providers, + ); + internalCoreModule.addExportedProviderOrModule(name); + return newWrapper; + } }