Circular dependencies and request-scoped providers | NestJS | Page 1

kind geyser Mar 17, 2023, 7:42 PM

#

Hi everybody 👋

I'm new to NestJS and to this Discord community. It's my understanding that this is the best place to ask questions, but if not, please let me know!

So here goes: I recently opened an issue about issues with circular dependencies and request-scoped providers. You can find the issue here: https://github.com/nestjs/nest/issues/11294

I understand that circular dependencies are an architectural smell and should be avoided and that request-scoped circular dependencies are even worse. That being said, we're looking to migrate a legacy Node.js app to NestJS incrementally so we can't avoid running into situations like these.

Also, I understand the technical challenges with circular dependencies and the non-determinism that's present when it comes to instantiate a provider that has a circular dependency.

That being said, these challenges tend to manifest themselves when relying on an instantiation order in the provider constructors since the constructor of one provider might've run before the other. What I don't understand is why this would impact a controller's method. I would have imagined that when a request comes in NestJS would be able to instantiate all request-scoped providers (in a non-deterministic order, that's ok), and then call the relevant controller methods. Is this just not possible? How come in the issue I linked above this is fixed by having forwardRef in the other direction? It seems that NestJS should be able to handle this without issue but my guess is that I'm missing something or misunderstanding how NestJS instantiates providers during a request and how that affect controller method calls. I would really appreciate if somebody familiar with NestJS internals could shed some light here 😸

balmy dock Mar 17, 2023, 7:45 PM

#

Give me about an hour and I'll be back at a place where I can dive in a bit on this

kind geyser Mar 17, 2023, 8:06 PM

#

That's great! Thanks @balmy dock. By the way, something that's peculiar is that if the forwardRef is removed so there's still a circular dependency and both providers are request-scoped, the issue is solved! So it seems that adding the forwardRef introduces the issue, but forwardRef is resolved at app initialization time, not request time, so it seems that simply adding forwardRef shouldn't break it, or put differently, it seems that the error should happen due to the circular reference, regardless of the forwardRef, but that doesn't seem to be the case. Happy to provide a video if this description is too hard to follow 😅

balmy dock Mar 17, 2023, 8:41 PM

#

So, first question here: why are you using forwardRef when there isn't a circular dependency?

kind geyser Mar 17, 2023, 9:51 PM

#

There's a circular dependency at the module level (i.e., JS imports) so the value of the import is undefined as Node.js parses. Since forwardRef evaluates lazily, it fixes that problem. This is similar to how GraphQL field resolvers from graphql work, you can use a field config object, or a thunk to make it lazy if you have circular dependency in types.

balmy dock Mar 17, 2023, 9:55 PM

#

Care to explain the circular dependency here? I don't see it. AppModule imports both A and B services, but neither of those back-import AppModule. A service imports B service, but B service doesn't import A service. AppController imports A service, but again, A service does not import AppController

balmy dock Mar 17, 2023, 10:20 PM

#

Apologies if I accidentally closed this. Must've hit the wrong keys while swapping context

kind geyser Mar 18, 2023, 5:02 AM

#

I'm sorry for the delayed response, I had to step away from my computer and I got back just now.

The circular dependency is not present in the reproduction since that wasn't necessary to reproduce the bug. In the latest commit, though, I've introduced a circular dependency at the module level so you can see what I mean: https://github.com/migueloller/nest-forward-ref-repro/commit/326705b5d82865fbb14a8d153516cf0169561b39

Now, removing the forwardRef doesn't fix the issue (as it did before) and instead, the value of BService can be seen as undefined in AService.

So in the original repro, regardless of the circular dependency, introducing forwardRef breaks things, which is confusing to me. In the latest version I introduce a circular dependency to show the motivation behind using forwardRef and show the bug still happens.

Apologies if I accidentally closed this. Must've hit the wrong keys while swapping context

No worries! I really appreciate you taking the time to respond here and explain 😄

#

From adding some logs, it seems that NestJS calls the forwardRef callback when initializing the application, so by the time a request comes in, all providers should be known by NestJS, even ones with circular dependencies. Then, it would seem, NestJS should be able to instantiate all dependencies, in arbitrary order (so relying on order in constructors is a big no-no), and then call controller methods, at which point all dependencies should be done constructing, resulting in no issues

#

Or at least that's what I'm assuming should be possible and where I'm looking for some help in understanding

balmy dock Mar 18, 2023, 5:13 AM

#

So, what you've created are circular file imports, which are slightly different than circular dependencies, right? In a circular file import, you just have file a imports file b imports file a. Most of the time because on a part of a file a needs file b and a part of file b needs file a, and those parts can usually be split into new files to get away from this circular structure

With a circular dependency, class A needs class B to be able to be created, but class B needs class A to be able to be created as well. All circular dependencies are circular file imports, but not all circular file imports are circular dependencies

kind geyser Mar 18, 2023, 7:28 PM

#

Yeah, that makes sense. I'm aware that this isn't ideal but it's something that currently exists in the legacy codebase and we're trying to work with incrementally.

Independent of the circular file imports, it seems that adding a forwardRef shouldn't break things, right? Even before I introduced the circular import, without a circular dependency or circular file imports, the issue was happening

#

Looking at the original repro (without the circular file import) there's a problem by just introducing forwardRef. It seems that shouldn't be happening?

cerulean delta Mar 18, 2023, 11:36 PM

#

yeah this seems to be a bug to me.

#

it happens on v8 as well

kind geyser Mar 18, 2023, 11:48 PM

#

That's what I was thinking

balmy dock Mar 19, 2023, 12:12 AM

#

I need to look into some of the source code to help confirm why this is happening, but I'm not 100% sure it's a bug

balmy dock Mar 19, 2023, 1:17 AM

#

So, here's part of it: https://github.com/nestjs/nest/blob/master/packages/core/injector/injector.ts#L472

#

That asynchronous resolution is why you need process.nextTick()

#

And the "bug" is introduced because forwardRef was added. When that is used, Nest __must__assume there will be another one (at least) to go with it and will go into the asynchronous resolution via the callbacks created

kind geyser Mar 19, 2023, 4:48 AM

#

Interesting. So, would it be possible to wait until that next tick (or asynchronous resolution) before calling the controller method to avoid the "bug"?

balmy dock Mar 19, 2023, 4:49 AM

#

Not really, because this async method is called without an await to ensure that everything flows forward as best as it can

kind geyser Mar 19, 2023, 4:52 AM

#

Got it. Do you mind expanding on why NestJS must assume there will be another forwardRef?

Really appreciate you explaining all this by the way, I know it's something you don't have to do

balmy dock Mar 19, 2023, 6:02 AM

#

Well, if you're using a forwardRef it should be because there's a circular dependency and if there's a circular dependency then there is at least a pair of classes that are dependent on each other, maybe more in the chain, but at least two. That's why if there's one forwardRef in the class's constructor's metadata, Nest assumes there will be at least one more

kind geyser Mar 19, 2023, 6:23 AM

#

That makes sense. Is there another mechanism, other than forward ref to inject a provider, but do so in a way that doesn't break when there's circular imports? For example, graphql allows using a thunk for field configs. This is very similar to forwardRef but it seems forwardRef was designed for circular dependencies, not circular imports.

Perhaps forwardRef could work for both, but that doesn't seem to be the case at the moment

balmy dock Mar 19, 2023, 6:48 AM

#

Is it possible to re-architect the files to not need the circular imports? That would be the ideal solution

#

The thing about graphql's forward ref type thing is that it's not making the classes along with possibly injecting a request object every request. It's something that happens at the start of the server due to how decorators work and only needs to be calculated that single time. That's the big difference here

#

Even with regular forward refs (non-request scoped ones) the calculation only happens once like the graphql check. It's only when the REQUEST gets added that this becomes an issue

kind geyser Mar 19, 2023, 2:51 PM

#

It is possible to make this change, but that would require a lot of work upfront and make it harder for us to start using NestJS as soon as possible.

What I’m not sure I understand is: if it works without forwardRef and forwardRef is only called at application boot time (not per request, I added a log to confirm this) then it shouldn’t behave any differently when adding forwardRef because the request is resolved a very long time after application boot time

#

Or put differently, why does NestJS not unwrap the forward ref when the application is initialized and then treat it just like any other @Inject once it has been resolved?

#

And given that the forwardRef callback is called only during application init, it seems this is possible?

#

And just to be clear, when I talk about GraphQL's solution, I don't mean NestJS' GraphQL features, but plain GraphQL.js: https://github.com/graphql/graphql-js/issues/373#issuecomment-215913280

#

Regardless of whether a provider is request-scoped or not, the following is logged during app initialization and not for each request (see screenshot)

So why should it behave differently than a normal @Inject(BService) by the time a request comes in? NestJS should already have called the forwardRef by then and know about the provider exactly as if forwardRef wasn't used

balmy dock Mar 19, 2023, 4:30 PM

#

kind geyser Regardless of whether a provider is request-scoped or not, the following is logg...

I believe it's not about the reading of the forwardRef, but rather the resolving the value for it so that the forwardReffed class can properly be loaded. It's not like Nest just replaces the this.request property on each request, no, it creates a new instance of the class each time. So while the providers map is there and populated, and the metadata of the class type is known, Nest can't guarantee how long it'll take for the class to be instantiated, and as A depends on B and vice versa, the callback is necessary so that when A finishes being created, with a placeholder for B, B can be updated with the true value of the A instance, and then A can be updated with the truly value of the B instance.

kind geyser Mar 19, 2023, 5:11 PM

#

Hmm, I’m not sure I follow. Given the forwardRef callback is only called at app initialization time, from NestJS’ perspective it has the provider just as if forwardRef wasn’t used by the time the request comes in. The fact that removing the forwardRef fixes the issue shows the point, and there’s not even a circular dependency.

It’s what @cerulean delta pointed out above

#

At app boot time NestJS will traverse the dependency graph and get a reference to each value. If there's a circular import then forwardRef can be used to avoid undefined since NestJS will call forwardRef lazily after JS modules are evaluated.

Then, once the app is booted, NestJS can call the forwardRef callback and get all values. Now, if there's a circular dependency, the order of instantiation is non-deterministic because when A depends on B and B on A then you need to start somewhere and give A or B a "partial" version of its dependency while it's resolved.

Now, here's the rub, when a request comes in, NestJS has already called fowardRef so if we have @Inject(forwardRef(() => BService)) or @Inject(BService) by the time the request comes in, NestJS has already called () => BService so it's equivalent to @Inject(BService) in terms of what NestJS knows. But, even though NestJS has the same amount of information, @Inject(BService) works while @Inject(forwardRef(() => BService)) doesn't

balmy dock Mar 19, 2023, 5:18 PM

#

Right. So when the request comes in, Nest is instantiating BService and then hooking it back into AService asynchronously using that callback that I linked to above. It doesn't matter that we already know that it's BService, the class, Nest isn't resolving that metadata again, but it is making a new instance of the class, and that's why the process.nextTick() was required, to ensure that that promise fully resolved behind the scenes

kind geyser Mar 19, 2023, 5:21 PM

#

I think I understand that. I guess what I'm trying to get to here is that perhaps that doesn't have to be asynchronous? The order is already non-deterministic so merging the prototypes could happen synchronously before the controller method gets called. Making it asynchronous doesn't make it any more safe or deterministic, it results in this "bug"

#

It seems that what's being "awaited" is the instance host so that request info is available to the providers, but once that's done, it seems that the providers can be instantiated and prototypes can be merged, before calling controller methods.

It feels like circular dependencies should result in non-deterministic order in a provider's constructor, but not in a controller's methods. For example, I don't expect to be able to use BService in AService's constructor, but I expect to be able to use it in AService.hello() even if it's a circular dependency

#

And again, I apologize that I'm probably lacking all the relevant context here. If I knew the NestJS codebase I would happily try to provide more detail or contribute. Or perhaps I might see something I'm not.

But based on my understanding of JS and having run into circular dependency issues before, it seems that non-determinism of instantiation order should only be a problem in constructors, and not class methods

balmy dock Mar 19, 2023, 5:26 PM

#

kind geyser It seems that what's being "awaited" is the instance host so that request info i...

For what it's worth, I did actually take your example and truly made A and B circularly dependent on each other, and when both were forwardReffed properly even with the REQUEST scope, I didn't need theprocess.nextTick()

kind geyser Mar 19, 2023, 5:32 PM

#

Yeah, that's right! If both sides use forwardRef then it will work without issues. In our legacy Node.js app we don't actually have a circular dependency and instead have circular imports so we have to use forwardRef so that the imported binding isn't undefined when calling @Inject. But it doesn't make sense to use forwardRef (or @Inject) in the other provider since there is no circular dependency.

In fact, we can add a dummy dependency and make it circular so that we don't have to use process.nextTick() and given this would be a temporary thing we are open to using this method or others to continue our migration to NestJS.

That being said, I still thought it would be a good idea to report this behavior since I find it somewhat weird that adding forwardRef can break things. It seems that adding forwardRef should only fix existing circular dependency issues, but adding it shouldn't cause problems. If NestJS was able to resolve the dependency and things were working before introducing forwardRef then adding forwardRef shouldn't cause a problem (specially since the metadata is resolved at app init time, just as when forwardRef wasn't there and things were working)

#

If I could somehow summarize why this feels like a "bug" is because of that: I expect adding forwardRef to fix an existing circular dependency issue, but if things were working and I added forwardRef, I wouldn't expect them to stop working

balmy dock Mar 19, 2023, 5:34 PM

#

Would it be possible to show why exactly you're using forwardRef? I Feel like a lot of this is coming down to the use of forwarRef on only part of the circular chain, and that a better solution would be to move the circular part to a separate file so that it can not have to use forwardRef in the first place

kind geyser Mar 19, 2023, 5:40 PM

#

I wouldn't worry about us being able to move forward since this issue has clear workarounds either by fixing the circular import, adding a forwardRef in the other "side" if there is a circular dependency, or using process.nextTick()

But as the minimal reproduction shows, it's still an issue that can happen when using forwardRef to avoid circular imports. Again, you can argue that the solution is to fix the circular imports problem and that NestJS will never support forwardRef unless it's used in both "sides" and I would be ok with that.

I just wanted to know whether this behavior of things not working when using forwardRef in just one side for request-scoped providers was a bug or if it was a conscious design decision. And if it was a conscious design decision I wanted to understand why since it seems that there's isn't necessarily a technical limitation—that I'm aware of—that would prevent this from working. Again, since the non-deterministic nature of circular dependencies should be a problem in constructors not in methods, which should have all dependencies resolved by the time they're called by having instantiating all providers

#

And I just want to repeat again, I really appreciate you taking all this time and having the patience to talk about this with me. I'm just very curious and really want to understand NestJS well before we rewrite our backend in it and I'm very grateful that you guys have built and maintained this OSS technology. At the end of the day, if we need to work around this temporarily to be able to leverage NestJS, that's ok. This is a minimal roadblock

balmy dock Mar 19, 2023, 5:46 PM

#

You can argue that the solution is to fix the circular imports problem and that NestJS will never support forwardRef unless it's used in both "sides" and I would be ok with that.
This will probably be the case, to be honest

I just wanted to know whether this behavior of things not working when using forwardRef in just one side for request-scoped providers was a bug or if it was a conscious design decision. And if it was a conscious design decision I wanted to understand why
I wish I could give a "why" to this to be honest. Most of the code was written before I started working with the framework, so I'm figuring everything out as I read through and untangle everything in my own mental model

#

I really appreciate you taking all this time and having the patience to talk about this with me
Honestly, it's good for me to have to deep dive this stuff every now and again and figure out what was written a while back

kind geyser Mar 19, 2023, 6:24 PM

#

This will probably be the case, to be honest
Totally fine!

I wish I could give a "why" to this to be honest. Most of the code was written before I started working with the framework, so I'm figuring everything out as I read through and untangle everything in my own mental model
Regardless, I appreciate you helping out! Maybe some day your or I will understand this enough so we can get an answer as to "why" 😄

Honestly, it's good for me to have to deep dive this stuff every now and again and figure out what was written a while back
❤️

#Circular dependencies and request-scoped providers