Hi there, I read this PR this afternoon https://github.com/linagora/james-project/pull/2398 and I found a recurring problem here: https://github.com/linagora/james-project/pull/2398/files#diff-34220ca9b8dffd3a398bb2b3ce17c81eR108
As it's a problem that occurs frequently and pass reviews easily, I think that it deserves an explanation about what is the expected design because doing such errors probably proves that the explanations are lacking. Let's first explore the problem. Problem 1. Years ago, James was using Spring as the only way to do dependency injection. And it provides a nice feature call @PostConstruct. This annotation allows to start your application it two phases: the first is injection, the second is components initialization. Guice doesn't offer that feature out of the box. So we tried to implement that feature we some smarts use of Guice features (and as everybody knows, trying to be smart is very often a very good way to make things too hard understand). To implement this @PostConstruct substitute, we created some classes extending ConfigurationPerformer that would just call whatever methods are useful to initialize components. These ConfigurationPerformer classes are then added to a Guice multibinder and as a last step, in server start method, we call these ConfigurationPerformers. It worked for some time and then it failed. Problem 2. Initialization is supposed to respect dependencies ordering. So calling ConfigurationPerformers can fail because an object of type A is calling an object of type B before B is initiliazed. The solution is to listen to type injection to build a List (a List keep insertion ordering) of the types that are injected in the order they are created. As the injection framework creates objects with respect to the dependency hierarchy, we then end up with the sequence we need to respect for the initialization phase. But this only give us the List of objects to initialize and not the objects that perform the init. So we iterate the List and for each element we try to find the matching ConfigurationPerformers. For this to work, every ConfigurationPerformer has to declare what are the classes it initializes with the famous forClasses method. For each ConfigurationPerformer, we finally call the init method. For the specific situation linked at the top of this email, the ConfigurationPerformer declares MailboxIndexCreator as the type it initializes. But MailboxIndexCreator is not injected anywhere, so it will be created without any specific ordering. The truth is: we want indices creation to happen before we start objects that depend on an ElasticSearch connection, otherwise, the indices may not exist in time for this object to work. So in this case, the solution is quite simple: we probably have to declare RestHighLevelClient or something related in the forClass of the ConfigurationPerformer. One last point that we have to look at is the interaction with StartupCheck: do we want to try to create indices before checking ElasticSearch version? (I guess not). Thanks for reading and don't hesitate to ask if you need some more explanations. -- Matthieu Baechler --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
