Skip to content

Conversation

mag123c
Copy link
Contributor

@mag123c mag123c commented Aug 26, 2025

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

When transient services are injected into request-scoped controllers, the onModuleInit() lifecycle hook is called during application bootstrap even though the service instance has not been created yet. This results in:

  • The lifecycle hook being called on a "phantom" instance
  • Potential errors if the hook relies on constructor initialization
  • Inconsistent behavior with the expected transient service lifecycle

Issue Number: #15553

What is the new behavior?

The fix adds an instance existence check before returning transient instances for lifecycle hook processing. Now:

  • Lifecycle hooks are only called on transient services that have been properly instantiated
  • Transient services in request-scoped controllers won't have their hooks called during bootstrap
  • The behavior is consistent with the transient scope's intended lifecycle

The change is minimal and defensive - it simply adds && !!item.instance to the existing filter condition in getStaticTransientInstances().

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Tests were not added because:

  • This is an edge case that requires complex integration testing (request-scoped controller + transient service)
  • The fix is a simple defensive check that doesn't change existing behavior
  • All existing tests (1783) pass without regression

@coveralls
Copy link

coveralls commented Aug 26, 2025

Pull Request Test Coverage Report for Build 2171e1b3-d8d9-4560-88c0-60cf552b1da6

Details

  • 3 of 3 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 88.737%

Totals Coverage Status
Change from base Build e0294e19-6823-4ec3-9f07-4aa875fdaf3b: 0.003%
Covered Lines: 7272
Relevant Lines: 8195

💛 - Coveralls

@mag123c
Copy link
Contributor Author

mag123c commented Aug 26, 2025

The WebSocket test failure seems to be a flaky test (connection cleanup timing issue).

@albert-mirzoyan
Copy link

This change does not affect the behavior. The item.instance is not nullish, it is an empty object, but still an object nonetheless in both request -> transient and singleton -> transient -> transient scenarios.

In the instantiateClass method, the decision to call a constructor or not is sophisticated, but still insufficient to handle deep transient cases such as singleton -> transient -> transient. It seems like this decision, or ability to perform a similar inquirer-related check, should be carried over and applied in getStaticTransientInstances.

@mag123c
Copy link
Contributor Author

mag123c commented Aug 26, 2025

Thank you @albert-mirzoyan for the insightful review! You're absolutely right.

The issue is that item.instance is not nullish but an empty prototype object created by Object.create(this.metatype!.prototype) in cloneTransientInstance(). This happens when the constructor hasn't been called yet.

I've updated the fix to check item.isResolved instead, which properly indicates whether the instance has been fully instantiated:

  • isResolved = false: Only prototype object exists, constructor not called
  • isResolved = true: Constructor has been called, instance is ready for lifecycle hooks

I think This handles all scenarios including complex dependency chains like singleton → transient → transient.

@albert-mirzoyan
Copy link

Sadly, this return item && item.isResolved; also won’t change anything, because isResolved is true even when the instance is incomplete. So no, I think the issue is deeper. I’m still investigating and not yet ready to pinpoint it exactly.

@mag123c
Copy link
Contributor Author

mag123c commented Aug 26, 2025

@albert-mirzoyan
You're absolutely right. After investigation, I found that isResolved = true is executed unconditionally, even when the instance hasn't been properly initialized:

  1. When isInContext = false, both if blocks (lines 829-844) are skipped
  2. The instance remains as an empty prototype object from Object.create()
  3. But isResolved is still set to true at line 845
  4. Therefore checking item.isResolved doesn't solve the problem

This explains why lifecycle hooks are still being called on incomplete transient instances in complex scenarios like singleton → transient → transient.

The issue is indeed deeper than initially thought. A few potential approaches:

  • Add isInContext check logic to getStaticTransientInstances
  • Track actual constructor invocation with a new flag
  • Differentiate between prototype-only vs fully instantiated instances

cc @kamilmysliwiec @micalevisk - would appreciate your input on the preferred approach here. Should we pursue one of these solutions or investigate further?

@kamilmysliwiec
Copy link
Member

Track actual constructor invocation with a new flag

Should be as easy as adding a new attribute to the InstanceWrapper class and setting it to true upon the class ctor call

@mag123c
Copy link
Contributor Author

mag123c commented Aug 26, 2025

@kamilmysliwiec
Thank you for the guidance! I've implemented with the isConstructorCalled flag as you suggested.

@albert-mirzoyan
Copy link

Thank you @mag123c

With the isConstructorCalled change, the faulty onModuleLoad calls are gone, so the request -> transient case seems to be working as expected.

The singleton -> TransientService -> DeepTransientService case is still incorrect, but this could potentially be tracked separately. In that scenario, although the symptoms were similar, the resolution is different: we would expect the constructor of DeepTransientService to be called. Currently, onModuleLoad is suppressed (isConstructorCalled) on DeepTransientService, and we still end up with a property in TransientService referencing an unconstructed DeepTransientService object.

The issue might be in isStatic, which prevents nested DeepTransientService instances from being constructed.

@mag123c
Copy link
Contributor Author

mag123c commented Aug 27, 2025

@albert-mirzoyan
I've created a test case for the scenario (singleton → transient → transient):

describe('Nested Transient Services', () => {
  it('should properly instantiate all nested transient services in singleton -> transient -> deepTransient chain', async () => {
    const constructorCalls: string[] = [];
    const onModuleInitCalls: string[] = [];

    // Deep transient service (innermost)
    @Injectable({ scope: Scope.TRANSIENT })
    class DeepTransientService implements OnModuleInit {
      public value = 'deep-value';

      constructor() {
        constructorCalls.push('DeepTransientService');
      }

      onModuleInit() {
        onModuleInitCalls.push('DeepTransientService');
      }

      getValue() {
        return this.value;
      }
    }

    // Middle transient service
    @Injectable({ scope: Scope.TRANSIENT })
    class TransientService implements OnModuleInit {
      constructor(public deepService: DeepTransientService) {
        constructorCalls.push('TransientService');
      }

      onModuleInit() {
        onModuleInitCalls.push('TransientService');
      }
    }

    // Singleton service (outermost)
    @Injectable()
    class SingletonService implements OnModuleInit {
      constructor(public transientService: TransientService) {
        constructorCalls.push('SingletonService');
      }

      onModuleInit() {
        onModuleInitCalls.push('SingletonService');
      }
    }

    @Module({
      providers: [SingletonService, TransientService, DeepTransientService],
    })
    class TestModule {}

    const moduleRef = await Test.createTestingModule({
      providers: [SingletonService, TransientService, DeepTransientService],
    }).compile();

    await moduleRef.init();

    const singletonService = moduleRef.get(SingletonService);

    // Verify all constructors were called
    expect(constructorCalls).to.include('DeepTransientService');
    expect(constructorCalls).to.include('TransientService');
    expect(constructorCalls).to.include('SingletonService');

    // Verify services are properly instantiated and connected
    expect(singletonService).to.exist;
    expect(singletonService.transientService).to.exist;
    expect(singletonService.transientService.deepService).to.exist;

    // Verify the deep service is functional (not just a prototype object)
    expect(singletonService.transientService.deepService.getValue()).to.equal(
      'deep-value',
    );
    expect(
      singletonService.transientService.deepService.constructor.name,
    ).to.equal('DeepTransientService');

    // Verify lifecycle hooks were called appropriately
    // With our fix, transient services in this chain should still have their hooks called
    // when they are properly instantiated
    expect(onModuleInitCalls).to.include('SingletonService');

    await moduleRef.close();
  });

  it('should not call lifecycle hooks on prototype-only transient instances', async () => {
    const onModuleInitCalls: string[] = [];

    @Injectable({ scope: Scope.TRANSIENT })
    class TransientService implements OnModuleInit {
      onModuleInit() {
        onModuleInitCalls.push('TransientService');
      }
    }

    // This service won't be instantiated during bootstrap
    @Injectable({ scope: Scope.REQUEST })
    class RequestScopedService {
      constructor(private transientService: TransientService) {}
    }

    @Module({
      providers: [TransientService, RequestScopedService],
    })
    class TestModule {}

    const moduleRef = await Test.createTestingModule({
      providers: [TransientService, RequestScopedService],
    }).compile();

    await moduleRef.init();

    // The transient service should not have its lifecycle hook called
    // because it wasn't actually instantiated (only request-scoped service depends on it)
    expect(onModuleInitCalls).to.not.include('TransientService');

    await moduleRef.close();
  });
});

The test passes, showing that DeepTransientService is properly instantiated. The isConstructorCalled flag only controls lifecycle hooks, not instantiation itself.

However, I haven't extensively used transient services in production, so there might be edge cases I'm not considering. If you have a specific scenario where the DeepTransientService isn't being instantiated properly, could you share a reproduction? That would help me understand what I might be missing.

Is this the singleton → transient → transient pattern you were referring to, or is there a different configuration I should test?

@albert-mirzoyan
Copy link

@mag123c
Yes, your test case with a singleton provider as the root passes on my side as well. Thank you for checking that.
I should have been clearer in my description of the issue: it is Controller (Singleton) -> TransientService -> DeepTransientService that fails.
Here is a slightly modified version of your test case.

import { Controller, Injectable, Module, OnModuleInit, Scope } from "@nestjs/common";
import { Test } from "@nestjs/testing";

describe('Nested Transient Services', () => {
    it('should properly instantiate all nested transient services in singleton -> transient -> deepTransient chain', async () => {
      const constructorCalls: string[] = [];
      const onModuleInitCalls: string[] = [];
  
      // Deep transient service (innermost)
      @Injectable({ scope: Scope.TRANSIENT })
      class DeepTransientService implements OnModuleInit {
        public value = 'deep-value';
  
        constructor() {
          constructorCalls.push('DeepTransientService');
        }
  
        onModuleInit() {
          onModuleInitCalls.push('DeepTransientService');
        }
  
        getValue() {
          return this.value;
        }
      }
  
      // Middle transient service
      @Injectable({ scope: Scope.TRANSIENT })
      class TransientService implements OnModuleInit {
        constructor(public deepService: DeepTransientService) {
          constructorCalls.push('TransientService');
        }
  
        onModuleInit() {
          onModuleInitCalls.push('TransientService');
        }
      }
  
      // Singleton service (outermost)
      @Controller()
      class SingletonController implements OnModuleInit {
        constructor(public transientService: TransientService) {
          constructorCalls.push('SingletonService');
        }
  
        onModuleInit() {
          onModuleInitCalls.push('SingletonService');
        }
      }
  
      @Module({
        controllers: [SingletonController],
        providers: [TransientService, DeepTransientService],
      })
      class TestModule {}
  
      const moduleRef = await Test.createTestingModule({
        controllers: [SingletonController],
        providers: [TransientService, DeepTransientService],
      }).compile();
  
      await moduleRef.init();
  
      const singletonService = moduleRef.get(SingletonController);

      // Verify all constructors were called
      expect(constructorCalls).toContain('DeepTransientService');
      expect(constructorCalls).toContain('TransientService');
      expect(constructorCalls).toContain('SingletonController');

      // Verify services are properly instantiated and connected
      expect(singletonService).toBeDefined();
      expect(singletonService.transientService).toBeDefined();
      expect(singletonService.transientService.deepService).toBeDefined();


      // Verify the deep service is functional (not just a prototype object)
      expect(singletonService.transientService.deepService.getValue()).toBe(
        'deep-value',
      );
      expect(
        singletonService.transientService.deepService.constructor.name,
      ).toBe('DeepTransientService');

      // Verify lifecycle hooks were called appropriately
      // With our fix, transient services in this chain should still have their hooks called
      // when they are properly instantiated
      expect(onModuleInitCalls).toContain('SingletonService');
  
      await moduleRef.close();
    });
  });

@mag123c
Copy link
Contributor Author

mag123c commented Aug 28, 2025

@albert-mirzoyan
This PR fixes the lifecycle hook issue from #15553
The constructor instantiation problem in Controller → Transient → Transient chains seems to be a separate issue.
Should we track it separately or here??

cc @kamilmysliwiec

@kamilmysliwiec
Copy link
Member

The constructor instantiation problem in Controller → Transient → Transient chains seems to be a separate issue.

Not 100% sure but I believe this has already been addressed here #15469

@albert-mirzoyan
Copy link

@kamilmysliwiec

I have tried the test case with NestJS 11.1.6 containing #15469 . It still fails in the same way. Should I open a new issue for this?

@kamilmysliwiec kamilmysliwiec merged commit 86094c6 into nestjs:master Sep 16, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants