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

feat: support asynchronous initialization of services #126

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

flux627
Copy link

@flux627 flux627 commented Apr 11, 2020

There are some very common use-cases requiring asynchronous initialization of class instances, e.g. database connection on a repository. This PR enables this via opt-in non-breaking features, without having to do said initialization outside of typedi.

This PR:

  • Defines abstract AsyncInitializedService class to be extended by client code
  • Adds Container.getAsync() and Container.getManyAsync() methods
  • Adds the asyncInitialization: boolean option to the @Service decorator's options object

Usage:

@Service()
class Engine extends AsyncInitializedService { // Extend this class
  ignition: string = "off";

  protected initialize(): Promise<any> { // Must implement this method
    // Do your async initialization here, returning a Promise
    return new Promise((resolve) => {
      setTimeout(() => {
        this.ignition = "running";
        resolve();
      }, 0);
    });
  }
}

@Service()
class Car {
  constructor(public engine: Engine) {
  }
}

// Will recursively resolve all subclasses of AsyncInitializedService
const car = await Container.getAsync(Car);
console.log(car.engine.ignition === "running"); // true

As long as you extend AsyncInitializedService and implement initialize(), this will automatically be resolved when using Container.getAsync() and Container.getManyAsync().

If you would rather not extend this and want more control, you can set the asyncInitialization option on the @Service decorator to true and make sure that you set YourClass#initialized to a Promise from within your constructor. The following is equivalent to the above example, using this option instead:

@Service({ asyncInitialization: true })
class Engine {
  ignition: string = "off";
  initialized: Promise<any>;

  constructor() {
    this.initialized = this.initialize();
  }
  
  protected initialize(): Promise<any> {
    return new Promise((resolve) => {
      setTimeout(() => {
        this.ignition = "running";
        resolve();
      }, 0);
    });
  }
}

@Service()
class Car {
  constructor(public engine: Engine) {
  }
}

const car = await Container.getAsync(Car);
console.log(car.engine.ignition === "running"); // true

Considerations / Potential Problems:
Now that this can be called asynchronously and may defer execution at multiple places within the code, it is possible to get into weird states, especially if you don't await when calling the new methods. It is, for example, possible to instantiate a global more than once. Given that the use-cases are aimed at start-up initialization, this should be mostly a non-issue, however it's clearly better if we can avoid users shooting themselves in the foot. One way to mitigate this is through maintaining a mutex on each ContainerInstance, and throwing an error if a container attempts to be used while another method is still in progress.

Another idea is to opt-in to asynchronous Container instances, either on the global instance or any scoped instance. This way, the different getAsync and getManyAsync naming could revert to use the original get and getMany. Then, if we implement the mutex, we could actually wait until the container is free instead of throwing an error (without having to worry about dealing with the sync methods).

TODO:

  • Right now there's a lot of duplicate code in ContainerInstance but I don't currently see straightforward refactor strategy yet. Would like maintainer feedback here.
  • Potentially adding mutex based on maintainer feedback
  • More tests
  • Update documentation

@jefjos
Copy link

jefjos commented Jul 14, 2020

Hi, do you know when this PR will get merged? I'm looking for this feature or some workaround.

@NoNameProvided
Copy link
Member

NoNameProvided commented Jul 30, 2020

I wonder if the automatic calling of an initializing function should be introduced to sync version as well.

I like the PR but I am not experienced with this project so cannot decide whether the implementation is a good approach for the problem or not.

PS: Kudos for the well-written description, make understanding rationale a lot easier.

@slavafomin
Copy link

I think that the user of the service shouldn't be aware about whether the service is synchronous or asynchronous (otherwise it would break the encapsulation and changing the service behavior would be impossible in the future). I believe, if we want to support asynchronous services, we should change the entire API interface to be fully asynchronous.

Regarding the concurrency issues, the DIC should initialize the promise on the first request and then to return the same promise to all callers, this would prevent the service from being initialized multiple times and all callers will eventually receive the same instance.

@NoNameProvided
Copy link
Member

we should change the entire API interface to be fully asynchronous.

That has a big performance overhead compared to using the sync API. That would mean serious perf regression when injecting services in loops with a few thousands of elements.

@slavafomin
Copy link

@NoNameProvided I'm not sure that I understand your point, could you elaborate on this please?

If you are not using real async calls to initialize your services than all promises will be resolved synchronously. This will add some overhead due to indirection and additional function calls, but I think that performance hit will be almost unnoticeable.

@NoNameProvided
Copy link
Member

I'm not sure that I understand your point, could you elaborate on this please?
...
If you are not using real async calls to initialize your services than all promises will be resolved synchronously.

In the above comment, you mentioned that we should change the entire TypeDI API to async, which means everything needs to return a Promise.

Creating promises is not cheap, in fact, there is like a 1:10 performance hit with async functions. If someone requests an instance from TypeDI in a loop or for example for every request on a web server, that will definitely add up. Once we started down this path, the user will have to create extra sync functions just to be able to await the Service, so ther performance hit will be rather depth of functions x 10 which is huge.

A very bacic perf test:

let counter = 0;
 
async function asyncTest() { return 1 + 1; }
function test() { return 1 + 1; }

// call both of them a lot of time
// counter = asyncTest() vs counter = test()

image

@NoNameProvided NoNameProvided added the flag: needs discussion Issues which needs discussion before implementation. label Aug 9, 2020
@weoreference

This comment has been minimized.

@rafaell-lycan
Copy link

Perhaps another interesting and useful addition could be @AsyncService? 🤷‍♂️

@NoNameProvided NoNameProvided changed the base branch from master to develop January 9, 2021 13:58
@NoNameProvided NoNameProvided changed the title Support asynchronous initialization of services feat: support asynchronous initialization of services Jan 9, 2021
@theodorDiaconu
Copy link

theodorDiaconu commented Feb 2, 2021

Something about it feels off. But I have no reasonable argument. I would personally avoid having this kind of logic in my container, as an alternative, you could do:

class A {
  ready: Promise<boolean>;

  constructor() {
     this.ready = new Promise((resolve) => {
        await this.init();
        resolve();
     });
  }

  async init() { ... }
}

const a = container.get(A);
await a.ready;

@NoNameProvided
Copy link
Member

I am thinking in something like

// pseudo code not real names
Container.set({ type: MyAsyncClass, id: MyAsyncClass, async: true })
Container.set({ type: SyncDependingOnAbove, id: SyncDependingOnAbove })

await Container.waitForClassInit() // or Container.set also returns a promise which can be awaited

This "waitForClassInit" function should be callable as much as you like and it would simply check if is there any async service that has not been initialized yet and awaits if it finds any. If a dependent service is registered before the class is resolved, TypeDI will wait for the async class to get ready before creating the instance of the sync class. This way we can keep the entire current API to get resources and users can await services as it works best for them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag: needs discussion Issues which needs discussion before implementation.
Development

Successfully merging this pull request may close these issues.

7 participants