Merge pull request #15405 from hajekjiri/bug/fix-race-condition-in-dependency-resolution

fix(core): fix race condition in class dependency resolution

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;
+  });
+});

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",

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<void>;
+  private resolve: () => void;
+
+  constructor(targetCount: number) {
+    this.currentCount = 0;
+    this.targetCount = targetCount;
+
+    this.promise = new Promise<void>(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<void> {
+    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<void> {
+    this.signal();
+    return this.wait();
+  }
+}

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<T>(
+    return this.resolveComponentWrapper(
       moduleRef,
       token,
       dependencyContext,
@@ -462,7 +490,7 @@ export class Injector {
     return param;
   }
 
-  public async resolveComponentInstance<T>(
+  public async resolveComponentWrapper<T>(
     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<T>(
@@ -689,6 +710,7 @@ export class Injector {
       return this.loadPropertiesMetadata(metadata, contextId, inquirer);
     }
     const properties = this.reflectProperties(wrapper.metatype as Type<any>);
+    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<T>(
@@ -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;
           }

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(<any>barrier, 'resolve');
+  });
+
+  afterEach(() => {
+    // resolve any promises that may still be waiting in the background
+    (<any>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((<any>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;
+    });
+  });
+});

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;
       });
     });

packages/testing/testing-injector.ts

@@ -23,7 +23,7 @@ export class TestingInjector extends Injector {
     this.container = container;
   }
 
-  public async resolveComponentInstance<T>(
+  public async resolveComponentWrapper<T>(
     moduleRef: Module,
     name: any,
     dependencyContext: InjectorDependencyContext,
@@ -33,7 +33,7 @@ export class TestingInjector extends Injector {
     keyOrIndex?: string | number,
   ): Promise<InstanceWrapper> {
     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<T>(
+    moduleRef: Module,
+    instanceWrapper: InstanceWrapper<T>,
+    contextId = STATIC_CONTEXT,
+    inquirer?: InstanceWrapper,
+  ): Promise<InstanceWrapper> {
+    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<T>(
+    err: Error,
+    moduleRef: Module,
+    name: any,
+    wrapper: InstanceWrapper<T>,
+  ): Promise<InstanceWrapper> {
+    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;
+  }
 }