Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Http Request based DI #1376

Closed
QuestionAndAnswer opened this issue Dec 13, 2018 · 11 comments

Comments

@QuestionAndAnswer
Copy link

commented Dec 13, 2018

I'm submitting a...


[ ] Regression 
[ ] Bug report
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Expected behavior

I would like to have dependencies injection based on HTTP request (so, not JS event loop call). Currently in my project I'm having chain of factories which constructing class depending from incoming http request. It is really complicated to maintain them manually. I'm doing first iteration of it and may be doing something wrong still, but anyway, existing of such feature could provide better abstractions and development performance.

C# ASP.NET and Java Spring provides such functionality and we may use their concept for Nest. Current language capabilities is already enough (as I saw other issue here about request based DI, which you closed as async-hooks is still experimental), so there shouldn't be any technical limitations for it. The only thing that should be done first, is that we need to align approach how this should be done for most convenient use by developers.

I'm ready to create PR for this, but this needs to be aligned first. What do you think?

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

Hi @QuestionAndAnswer,
Let me explain this in detail. First of all, we should avoid comparison to C# and Java, their request processing model is completely different due to the multithreading, basically, language and platform-specific differences.

Anyway, let's focus on the main discussion point. Injection by HTTP requests (scoped by requests) might be useful in a few different cases, for example, in order to cache the data just within the single request (like GraphQL data loaders), for better logging (tracing requests IDs), for multi-tenancy (different DBs/external sources for specific requests) and so on.

In order to achieve this, we could simply create a dedicated container instance for each incoming request and immediately initialize it, which would lead to an instantiation of every existing provider scoped just for this certain request. However, the problem is that memory usage would grow very fast during the massive load because initialization of each module means that we'll create each provider within each module. Even though we could use WeakMap where the request could act as key and value as our container which makes it pretty straighforward to garbage collect this object once it's no longer used, it's going to heavily affect our application. Hence, we should look for another solution, let's say, instead of creating each module on each request, we could potentially lookup 1 controller and then, recursively, instantiate its dependencies (and nested dependencies of dependencies) to only load what is used for this request's context. The problem is, that it's going to create very painful side-effects. For example, there are providers (especially async providers) that shouldn't be initialized more than once, let's say, we have the async provider which responsibility is to load our configuration file/or establish a connection to the DB (which should be shared across each request in this example) and so on. Shall we initialize them once again? Absolutely not. Another thing is that tracing dependencies up to an injection tree from the one entry point (1 route defined in the controller), would not imply that we create providers that depend ON providers needed in this context (inverse dependency). Let's assume that our provider is a hub that exposes a stream that other providers can subscribe to. And creating a dedicated instance for each request doesn't fit this model either.

All these things that I have outlined above make that it's slightly tough to find a good balance between performance and usability. Choosing either one of these strategies would create drawbacks and decrease predictability. So how async-hooks could potentially help in solving this problem? Generally, what we could do, is to first distinguish dynamic providers and static providers. All dynamic providers would be wrapped in a Proxy that acts as a bridge between consumer and producer. On each call to a dynamic (static would remain the same) provider (getter, setter, whatever), the proxy would lookup WeakMap of cached instances based on the async ID (request ID in our case) and either return in (if exists) or create a new one (and put it in the cache) in the background. 🙂

@QuestionAndAnswer

This comment has been minimized.

Copy link
Author

commented Dec 14, 2018

@kamilmysliwiec,
I think you are trying to solve bigger task than we currently need. With the use of async-hooks you can solve task of transient scoped DI, but for http (Call\Execution Context) request scope we can use self generated id's (incremental counter as fastest and simplest one 🙂 )

You mentioned here the main point

the proxy would lookup WeakMap of cached instances based on the async ID (request ID in our case)

Which saying, that async id is just another level of IoC-container functioning granularity.

Moreover, we can have same working IoC-container algorithm with different scope id providers. Like, for now we can have only self generated http call ids for DI, and after async-hooks release different scope id provider, which will tag each IoC-container call more precisely.

In case of request scoped IoC-container we will not loose this context. Closures will save it. At least I see plenty of situations where it can be used and none where it will break something really bad. Like, we don't need to identify where this request come from (maybe except microservices communication, but this also should be possible), but instead use constructed classes.

Anyway, my point is that we already have everything that we need for request scoped IoC-container, which might improve development performance and cover majority of use cases. Transient and Request scoped DI are not intersecting each other, but completes single set actually. So, maybe it is better to take baby steps in this direction?

If you will give me some time, I'll try to provide example how I see this might be done. This weekend I think.

Spoiler.

@Module(
...
providers: [
    {
        scope: SCOPE.REQUEST,
        provide: Service1,
        useRequestFactory: (ctx: ExecutionContext) =>
        {
          return new Service1(
            ctx.switchToHttp().getRequest().params.p1;
          );
        }
      },
      {
        // Singleton scope should be used by default to not break backward compatibility
        scope: SCOPE.SINGLETON,
        provide: Service2,
        useValue: new Service2('singleton value')
      }
]
...
)
@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

I'm talking about request-scoped, not transient scoped DI (but it's true that transient scoped DI would be possible to solve with async-hooks as well).

So basically, what you have described above is also addressed in my post, specifically, here:

We could potentially lookup 1 controller and then, recursively, instantiate its dependencies (and nested dependencies of dependencies) to only load what is used for this request's context

Which is basically fine and pretty straightforward to achieve. We could just have another Map, where the key is a unique ID of the request, it's gonna take the controller, resolve its dependencies, either create their instances (and store in the request-container) or reuse existing ones depending on the scope.

In order to clarify, I fully agree up to this point! 😄

Let's think about the potential solution. First of all, the controller's methods are directly bounded to the route handlers, which means that we would have to create another layer here. It's, obviously, not a big deal. It might a proxy that just creates a new controller and return a new one for each request. Although, each controller may have some metadata associated with it, such as guards, pipes, and interceptors. All of them should be also configurable (to be more precise, their scope should be configurable). So we would create these objects (or reuse existing ones), based on the metadata, and afterward, create a final handler using closures (as we do right now) which again, gives us some additional overhead. Then, services and all dependencies. We need to instantiate deps up to the injection tree from the certain controller. Some providers might be request scoped, some could act as singletons. Nevertheless, if one provider depends on request scoped provider, it will be automatically transformed to the request-scoped provider as well (because we need to create a new instance to inject a specific request-scoped dependency, even if, without this particular dependency, provider would behave as singleton) - that is a very crucial thing to keep in mind too.

So this is everything that I'm talking about - predictability and a lot of super overhead. I think that we could do better, for example, by taking advantage of async hooks. Inspired by this issue, I started to experiment with this functionality yesterday and this is what I have achieved so far. I would love to know your opinion about it. 🙂

So basically (as I outlined in the first post), we could divide instances into static and dynamic (API to be discussed). For now, I just assumed that a dynamic provider looks like this:

@Injectable({ dynamic: true })
//// OR
{
  provide: Boom,
  useFactory: (asyncContext: AsyncContext) => {
        return new Boom(asyncContext);
   },
   inject: [AsyncContext],
   dynamic: true,
}

Certainly, instead of using dynamic we could use scope for example (to have room for other scopes that might be introduced in the future, for example, transient scope).

Now, just to explain how dynamic works. Dynamic provider is a sort of lazy, request-scoped provider. Nest would instantiate request-scoped class ONLY if it's gonna be used within this particular request, lazily. Let's say, you have UsersService which has 5 dependencies, but on GET / call only one of them is actually used, Nest would create only 1 dependency to reduce memory usage and make this process much faster. Same with exception filers for example, instance would be created ONLY if error was thrown. This prototype is built on top of async-hooks and proxies.

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

After a while of reflection, I believe that we could actually provide all these scopes at once, respectively: per-request (with immediate instantiation), singleton, and lazy/dynamic request-scoped and give a choice to the user :)

@QuestionAndAnswer

This comment has been minimized.

Copy link
Author

commented Dec 16, 2018

Second option I like more, and also giving my voice for scope name, as you said, for future options, and because it is more classical name in containers world (easier to understand).

Another point is that I believe that you don't need to introduce anything like dynamic mark at all (to provide lazy request scoped instantiation), as during run time you always have limited number of entry point from Nest to user specific code. These parts are controllers, guards, pipes, middlewares, interceptors and some other few things.

Which bringing us to the topic of unified approach of instance retrieval in all operations. I see this as the following:

image

On the left side we have nest instance container which has dependencies in it. They are marked in form f(x) - some function which showing type of dependency. f(1) - singleton (not depends from anything). f(r) - depends from request (or anything else). On the right side you can see typical hierarchy, with singleton parent, one singleton child and one request scoped child.
During execution all caller classes should request for appropriate dependency instance (controller, middleware and etc.) through this container, which has method to get instance of some class depending from some hash (calculated from request, requestId, async-hook id or anything else). In this case you will always get lazily instantiated modules tree, as construction would be triggered for those who is required during incoming request processing. All singleton dependencies will not be rebuild at all (reference to same object will be returned) and for request scoped, either already created instance or newly created instance will be returned.
More over this is easily cached through decorators implementation (SRP, SOLID, you know) in case if it is done like that.

That is how I see this should be done. I'm not so good at core code understanding right now, so you may correct me if this is already how it is working, but what I saw done in another way, which is completely enough for current IoC functioning, but hard for improvements.
As an example we can take a look on router-execution-context.ts (link with line number). This class is calling controller methods, as far as I've debugged. On this line you are making call to instantiated instance of controller with resolved arguments. I hope that this is the part that you described (that I wasn't confused) which need to be augmented, with Proxy usage and WeakMaps, for correct instance call retrieval.

BTW, why did you make containers system part of the core instead of delegating it to external libraries? Separation of such responsibilities would make core lighter (which is huge plus for microservices), reduces costs on maintenance and extensions (like this one 😉).

@marcus-sa

This comment has been minimized.

Copy link

commented Dec 17, 2018

So basically you want to end up with the same scope behavior as in Inversify?
https://github.com/inversify/InversifyJS/blob/master/wiki/scope.md

From my POV it's still bad practice to keep trying to reinvent the wheel, when there's already a battle tested IOC container out there.
Prove me wrong @kamilmysliwiec

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

@QuestionAndAnswer what you have described is basically finished in the feature/di-scopes branch :)

From my POV it's still bad practice to keep trying to reinvent the wheel, when there's already a battle tested IOC container out there.
Prove me wrong @kamilmysliwiec

I have answered this question thousands of times. It doesn't make sense to continue this discussion.

@QuestionAndAnswer

This comment has been minimized.

Copy link
Author

commented Dec 17, 2018

@kamilmysliwiec, great! When is release? 😃 Can I help with something?

Also, what about context injection? In your previous post you mentioned that this can be done via AsyncContext injection. Which alternatives did you chose from? Didn't you think about direct ExecutionContext injection?

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

6.0.0 has been published.

@dynamikus

This comment has been minimized.

Copy link

commented May 22, 2019

I know this issue is closed but does anyone have an actual working example using http request based di. I could not find any example/documentation anywhere. Any thing will help.
Thnx

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.