Asynchronous initialization of Feathers services

[bisubus]

This is real issue that I've recently had, but I guess this concerns the framework in whole. I think that it is natural for Feathers services to be asynchronously initialized. They can pick their configuration from asynchronous sources, or it can be something else.

In most cases synchronous application initialization is presumed, but I've encountered design problems when started using Sequelize.

While Mongoose does some good job on accumulating promises inside for initialization actions (connection, index creation), so queries are automatically chained to initial promises, Sequelize does nothing like that, e.g. when sync() is called.

I would expect that SQL database is ready at the moment when web server has been started, but when the application is defined in synchronous manner, it will not. And this is what happens with typical Feathers setup. In some cases async initialization may take several ms, in other cases it may take seconds, while the server will pollute logs with errors that should have been never happen normally.

Another problem is error handling. If there are uncaught errors during async service initialization, the reasonable move would be to halt app initialization and catch errors in a single place.

Here's a small example that explains the idea and is based on the structure proposed by Feathers generator template.

app.js:

const app = require('feathers')();

// THIS!
app.set('feathersServicePromises', []);

app
.use(require('body-parser').json())
.configure(require('feathers-hooks'))
.configure(require('feathers-rest'))
.configure(require('./middleware'))
.configure(require('./services'));

// THIS!
Promise.all(app.get('feathersServicePromises'))
.then(() => app.listen(3000), console.error);

services/index.js:

module.exports = () => {
    const app = this;

    app
    .configure(require('./foo-async-service')
    .configure(require('./bar-async-service')
    .configure(require('./baz-sync-service')
};

services/foo-async-service.js:

const sequelize = require('./common/sequelize');

module.exports = () => {
    const app = this;
    const fooModel = sequelize.model('foo');

    // AND THIS!
    const initPromise = fooModel.sync();
    app.get('feathersServicePromises').push(initPromise);

    app.use('/foo', require('feathers-sequelize')({ Model: fooModel  }));
    app.service('/foo');
};

The places of interest here are initPromise and Promise.all(...). It would be convenient to just return initPromise from service function, like it is done in hooks. And Promise.all(...) could probably be lazily executed with app object toPromise() method or promise getter property. So it could be

module.exports = () => {
    ...
    return fooModel.sync();
};

in service function, and

app
...
.configure(require('./services'))
.promise
.then(() => app.listen(3000), console.error);

in entry point.

I suppose that the pattern I've used for async initialization is already simple and clean, but it would be great if the framework would take this job.

My suggestion is to place this feature under consideration.

I would appreciate any thoughts on the subject.

[daffl]

I've been thinking about that a couple of times as well. The problem was that it will be a breaking change but we can probably get in with #258.

[bisubus]

@daffl #258 is huge. I've got my share of Hapi before, and it is totally different beast under the hood, the work on unification looks really tough. And Passport+Hapi initiative was abandoned years ago, so it is a fresh start.

As for configure alone, the thing described above is quite easy to implement and doesn't introduce breaking changes. Just not sure how many cases it will cover in larger perspective.

[idibidiart]

Well, after suggesting that service creation should return a function the other day, so as to signal to user that it's a dynamic thing, the next thought was to return a Promise. So no matter the order of service creation when you use .then() you can be sure the service is already initialized. However, you do the Promise scheme it's gonna be better than how it is at present :) A very minor nitpick, but has big consequence for DX imo.... Circular references between services are common place.

[bisubus]

@idibidiart I'm not sure if I understood correctly, does the code above fully cover the scenario you've described?

Probably service setup may return a promise, too?

[idibidiart]

@bisubus if I understand your proposal correctly (and I am knew to the Feathers and Express way of things), my concern is that A) it is a breaking change since the app.service function doesn't return a promise and B) slow startup time as you have to wait on all services to launch as opposed to starting the app and then yielding/awaiting until promise for a given service resolves before proceeding with service invocations.... if that makes any sense.

[bisubus]

@idibidiart In implementation above it was done for configure functions only (this is where services are usually defined), not for service itself. Currently returned value from configure function is ignored, this allows to return something (a promise) from there without breaking anything. I guess that a similar thing can be done in service setup functions, currently they don't return values, this can be used too.

This proposal in its current form doesn't slow or break anything. It just provides a promise of initialization, it can be either chained to make sure that everything is ready or ignored. In my case I chained app.listen(...) because it made perfect sense there.

[idibidiart]

i see what you mean! Thanks for the clarification. I think it makes sense to do it the way you've described including returning a promise from setup. What remains vague to me is where you say that app.service returns an instance of the service. I've gotten undefined when invoking it while the service is not ready. You're suggesting to wait on the setup promise to resolve prior to invoking app.service?

[bisubus]

@idibidiart Do you have a real-world example where app.service(..) can face a race condition?

I didn't mean that it will wait for setup promise in app.service(..) invocation, this would make app.service(..) to return a promise of a service, doesn't look good.

[idibidiart]

Yes I have an example I can describe and point you to.

I did not mean to wait for setup promise in app.service but to 'await' it from an async function (or use generator equivalent) before invoking the service methods. Won't require app.service to return a promise.

The example is where graphql service is configured before the services it depends on so when invoking app.service when graphql service is launched those services don't exist yet.

[idibidiart]

the thing is the dependency graph for services may have cycles (service A depends on service B and service B depends on service A) so we can't guarantee a linear dependency order, which is why I was thinking of waiting on setup promise resolving before proceeding with app.service invocation (again, if that makes any sense!)

[bisubus]

@idibidiart By 'graphql service is launched' you mean setup method in graphql service, right? Because other methods (CRUD) won't be called until the server starts (and this can be guaranteed with app.toPromise().then(() => app.listen(...))).

From my understanding of how Feathers service registration works, services will become available in the order in which they are defined, so the solution is to have app.configure(...) with dependency services definitions to be executed earlier than app.configure(...) with graphql service definition.

Yes, the thing you're describing is usually addressed with DI container. I like DI a lot, but here it looks like overkill to me. With Node modules, the proper order of service definition can be relatively easily maintained.

But you've touched unon interesting subject. There may be scenarios where it may be beneficial to have a promise of some particular service (returned from service setup), not just a resulting promise from Promise.all. I guess that this can be solved by storing initialization promises as a map, not as an array. This way the promises for separate services can be retrieved any moment, like app.service(...).toPromise().then(initializedService => ...). But I don't think that this can be used as a replacement for DI to solve the problem you've mentioned, returning a promise for yet undefined service will silently cause pending promise if there are circular dependencies.

[idibidiart]

The problem was in this project: https://github.com/advanwinkel/feathers-apollo-SQLite

Note how graphql is configured before the services that it uses in the resolvers. Moving app.service calls to the dynamic parts of the resolver fixes it. Correcting the configuration order fixes it too.

[idibidiart]

DI is too much agreed.

[idibidiart]

So if you look at src/services/resolvers.js here is what I mean:

`
// export async function
export default async function Resolvers(){

let app = this;

// used to have: let Posts = app.service('posts') but that returned undefined 
// because posts service was configured after the graphql service 
// So instead of that, I was thinking: 

// let Posts = await app.service('posts')
// which in this version does imply app.service returns a promise

return {

  // Type Resolvers (type, args, context)
  User: {
    posts(user, args, context){
     // Posts will be defined whereas it would be undefined without the promise/await
      return Posts.find({
        query: {
          authorId: user.id
        }
      });
    }
  },`

So elsewhere we'd have Resolvers().then(...)

Not sure what setup() does. In this case, the graphql "service" (aka graphqlExpress) does not have the Feathers service interface.

And what I meant by invocation-time circular dependency is like when Post resolver uses Users service and User resolver uses the Posts service. It wouldn't be a problem in this case because we're awaiting on Users service to resolve before calling it from Post resolver and awaiting on Posts service to resolve before calling it from User resolver. So I guess it's not a circular dependency between the Posts and Users service but a reciprocal relationship between the resolvers that depend on those services. Sorry to confuse.

Updated comments accordingly

I hope some of this makes some sense generally speaking despite my not fully grasping the way things work in Express and Feathers.

[bisubus]

@idibidiart Yes, I've understood you correctly then. This is solved by maintaining proper users-posts-graphql configure blocks order. This is a case exactly for DI, so it's not too much - just not for everyone. Most devs (myself included) would prefer to keep it simple if possible.

In the example you've shown graphql is not really Feathers service (they conventionally have setup method that contains all initialization code), so it doesn't fit the pattern I've described for setup. It is just a middleware that is defined in configure block (it could return a promise from there if needed).

The problem with promises of services that haven't been defined yet is that if Graphql depends on User, and User depends on Graphql (possibly unintentionally, and there may be more tiers), we will have classic circular dependency. This will result in pending promise with no error (while DI would throw CD error). Looks like antipattern to me.

This is why I think that this is a job for DI, not for promises alone. It would be nice to have DI container as an optional module, not necessarily a core part. If it could leverage existing initialization promises to perform async DI (don't remember if I've seen a thing like this somewhere else, but it is doable), this would be even better.

[idibidiart]

@bisubus

I believe I managed to confuse you in part. In the Feathers-GraphQL approach, Users service will never depend on GraphQL, as GraphQL is the only API end point and is the main entry.

What I was trying to solve for in the example code I shared is to ignore the configuration order because that is the anti-pattern: you cannot depend on order being linear, i.e. that their may be circular dependencies (DI is obvious solution there) but not in this case. But that is the reason I did not wish to resort to a fix that depends on configuration order.

By having a service return a promise I'm not solving the circular dependency case but I am solving another problem: wrong configuration order can lead to service being undefined and that presents a mystery to new comers who are not experienced enough with the way things work i Express/Feathers, so having to wait for the promise then proceeding with the remainder of the Resolver function (non-blocking thanks to async stack) we can assure that the Users or whatever service graphQL depends on is not undefined and that requests that may come to GraphQL before those services are defined will probably cause the server to throw an error because Resolvers.then(...) will not have setup the GraphQL service by then. But at least I can then easily tell what the issue is w/o having to understand the thing about configuration order.

It was a thought experiment. The learning outcome from it and this discussion (which forced me to think more about it) was that the simplest and easiest way is what @daffl had proposed and what i discovered while trying to fix it, which is to call the app.service(...) from within the resolvers, so that we have dynamic state. If services did have a circular invocation-time dependency, i.e. or more clearly stated: if they had 'reciprocal relation' in terms of service A calling service B and vice versa, that is not a problem. If the services were actually dependent on each other in terms of their instantiation then DI would be needed.

[bisubus]

@idibidiart I see your point. Thanks for explaining the example with Graphql, it adds some details.

Circular dependency was the most obvious pitfall in this situation, that's why I mentioned it. Also, there will be pending promise if there is no User or Post at all. If a thing like this should be done on framework level, these concerns cannot be ignored, because unexpected pending promise will be a PITA to debug.

[idibidiart]

The telling sign that the promise is pending is that the graphql service won't be there when we try to send it a request before the Feathers services it depends on are resolved (due to exported async Resolvers function returning a promise and being a dependency of GraphQL service, so we won't create the GraphQL service till the Resolvers promise resolves (unfortunate mixing of semantics here) We can find out which Feathers service is pending resolution with simple logging following each 'await' statement.

But I agree that is not a very formal way of dealing with the issue.

[bisubus]

@idibidiart The situation with pending promises can be always handled with Bluebird .timeout(), but I would consider this a hack.

[idibidiart]

Yeah like I said the telling sign in the example I shared is that the GraphQL service will not have been instantiated so Express/Feathers will return a 404. Easy to conclude that a promise fora service it depends on didn't resolve.

But I'm not high on that approach.

A declarative, event-driven state machine could be setup to handle the most complex startup/boot-up sequence. If there is interest I can propose an imperative version based on ES6 generators, which I believe node supports. I've done this sort of complex startup orchestration before in the UI.

[bisubus]

@idibidiart If you have something particular in mind, it would be nice to see it as a repo branch. There is possibly something that we could come up with.

[claustres]

Just to let you know that I had a similar problem and fixed it with I think a more simple approach, I overrode the configure() method like this:

app.configure = async function (fn) {
  await fn.call(this)
  return this
}

I also simply call the function given as parameter but await for it, so that you can have async operations in your callback. This means that if the lifecycle of your app is sequential you await app.configure() then app.listen(), otherwise you continue as before.

I hope this helps.

[bisubus]

@claustres Yes, this works as temporary solution but unfortunately, breaks API, app becomes unchainable. Hope we'll come up with something in v3.

[claustres]

Yes for sure it does not solve the backward compatibility issue...

[EJBroeders]

@claustres @bisubus Would there be something against it to make it a separate method configureAsync()? Then the backward compatibility is still guaranteed, and enables us to use those async patterns.

[claustres]

Yes it could be. I was also thinking of a way to make async functions chaining. Strictly speaking you can await in an expression so we could write something like this await (await app.configure()).configure() althought not really easy to read with many calls. Maybe a good chainable API would be something like this:

app
.configure()
.await() // This configure is async and we wait until it finishes
.configure() // This configure is sync or async but we don't wait for it
.configure()
...

The question is how to implement it ? Maybe by analysing the return the fn.call(this), if it is a promise (async) we store it and the next call to .await() in the chain will wait for it, otherwise it works as usual.

[bisubus]

@claustres Yes, that's the problem. configureAsync is unchainable and thus inconsistent with current API ( await (await ... ) ... doesn't count). The important problem that configureAsync could address is full control over awaited promises, so they could be chained manually in series or in parallel. E.g. when configure(bar) depends on configure(foo) promise but not on configure(baz). Same applies to service. I expect that the whole thing can be addressed by extending current API with no breaking changes. I'll give it a try soon.

[daffl]

I have not added this in v3 (maybe in 3.1) but this should be solvable with a fairly simple plugin:

module.exports = function() {
  const app = this;
  const oldConfigure = app.configure;

  app.ready = Promise.resolve();

  app.configure = function(fn) {
    app.ready = app.ready.then(() => Promise.resolve({
      callback.call(this, this);
      
      return this;
    }));

    return this;
  }
}

This would allow to return a promise (or use an async function) from am .configure callback and before starting your app you just await app.ready (the slightly finicky part is that you will have to wrap this in a self-executing async function since you can't use async/await at the module level).

[claustres]

I think I get your point although there is some typo in your code that might mess me up, it is similar to what I was thinking about with my await() except you force each configure to be async, store the current one in app.ready and update it with the next one when done. In this case await app.ready will launch the configure sequence.

[claustres]

Handle it using hooks seems great although I would add a new hook type to handle this use case (eg setup hooks). Indeed using regular hooks raises confusion IMHO because they seem to work like any other hook but actually they don't...

[bisubus]

Hook initiative looks interesting. Seems like there's no support of promises for middlewares. Is a middleware supposed to be wrapped with dummy service to enable promises?

[daffl]

This functionality will not affect how middleware runs. It is possible to register middleware in a setup hook after other asynchronous things have happened though:

setup: [ doAsyncStuff, context => context.app.use(errorHandler) ]

@claustres I was debating that as well, something looking like:

app.service('myservice').hooks({
  before: {},
  after: {},
  error: {},
  setup: {
    before(context) {
      await createDatabaseTableIfNotExists();

      return context;
    }
  }
});

app.hooks({
  before: {},
  after: {},
  error: {},
  setup: {
    before: createDatabaseTableIfNotExists(),
    after: startMonitoring(),
    error: restartInTenMinutes()
  }
});

It adds some inconsistencies when it comes to retrieving and registering hooks and doing codemods but it is more explicit than having some internal exceptions you have to know about.

[bertho-zero]

@daffl I have a question about this new way of handling the setup: what should go in the setup method and what should go in the hooks?

[daffl]

I think the same as with service methods. Anything that the service needs to work by itself should be implemented in setup. Anything that the application needs to set up the service should be added via hooks. I also just realized that there should be a service mixin that adds a setup method if there isn't one otherwise the hooks will blow up.

[bitflower]

Just to be sure: Would this officially support creating services at runtime anytime after start of the app (in my case user triggered) ?

[daffl]

This already works for websockets and is more of an Express than a Feathers limitation. You could create an updated notFound handler to be registered after all custom middleware but before your services and have it check the URL against the paths in app.services and only throw the NotFound error if nothing matches.

[KrishnaPG]

Is this working? The documentation of setup indicates that it returns a promise. Will the app wait for the promise or should we use the hooks as indicated here. Because the documentation of hooks does not indicate any hook for setup.

What is, currently, the right way to make the setup wait?

[germanp]

I'm think is missing returning a promise in app.configure(), that was the original cause for this issue. There is no way of aborting in case of fail during configuration (eg. database not available)

[N8th8n8el]

What's the status on this issue?
I am using version 4.3.0-pre.2 and service.setup methods don't seem to be asynchronous yet.

[daffl]

This will be part of v5 using the new hook system (#932).