Wait till all node_helper are started before finishing startup

[rejas]

In response to #2487 this implements a Promise.all for the node_helper start calls. Just a draft becuase I want to get feedback first....

[MikeBishop]

Two pieces of feedback:

• It might make sense to move the log line into the completion block, so that the message indicates when all modules have started rather than when all modules have been told to start. Or log in both places, to give some visibility into the time it takes for that to happen.
• Contrary to my original suggestion, Promise.allSettled() (Node 12.9+) might be more appropriate so that an error doesn't cause it to complete prematurely.

[rejas]

Thanks for the feedback @MikeBishop Now ready for real review ;-)

[codecov-commenter]

Codecov Report

Merging #2928 (afdaeec) into develop (a328ce5) will increase coverage by 0.24%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           develop    #2928      +/-   ##
===========================================
+ Coverage    63.82%   64.06%   +0.24%     
===========================================
  Files            9        9              
  Lines          293      295       +2     
===========================================
+ Hits           187      189       +2     
  Misses         106      106              

Impacted Files	Coverage Δ
js/app.js	60.33% <83.33%> (+0.66%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[sdetweil]

what about those that fail? are we expecting the module to alert the user somehow, and we say nothing? just thinking of debug

[rejas]

not sure how many module developers use async here at all ....

[sdetweil]

well, none in the start function.. cause it doesn't exist.. i use both promises and async where appropriate

[rejas]

a module developer could use async anyway, even if it is not used in the prototype method (just like the OP does). might be interesting to mention it in the docs for developers...

[MikeBishop]

FWIW, I wound up shipping mine using async anyway, and just putting the things that might take longer last. I haven't encountered any ill effects from my start-up routine completing a bit after the parent process thinks it did.

But @sdetweil makes an interesting point. In the current design, if a module throws an exception during start-up, does the server start-up fail entirely? If that's the desired behavior, then Promises.all() was the correct method to start with, and having it throw the first exception encountered and kill the program would provide a similar result.

[rejas]

Im currently on a trip, so no real way for me to check the current behaviour.
I would hope though that in case of an error the other modules start too and a meaningful error is shown somewhere (console or notification)...

[rejas]

In the current design, if a module throws an exception during start-up, does the server start-up fail entirely? If that's the desired behavior, then Promises.all() was the correct method to start with, and having it throw the first exception encountered and kill the program would provide a similar result.

Currently if a node_helper of a module throws an exception in the start method, all other node_helper after that dont get started due to:

"
[12.10.2022 22:49.47.556] [ERROR] Error: listen EADDRINUSE: address already in use 127.0.0.1:8080
"

As an example, use the default config and throw an exception in the newsfeed helper module. Calendar module wont start and show nothing...

With the changes here that will still happen. Why? Because the start method doesnt return a promise, and we have no try/catch anywhere in the startup sequence.

Do I like this behvior? No, since it confuses a user. If a module errors, only that module should error and not any other module that has nothing to do with it.

Two ways to fix it:

1. Add a try catch somewhere around where the server starts and handle it gracefully
2. Make the start method async (since it will then return a promise that can be handled with Promise.all...) and hope module developers change their method signature to use async too

Obviously 2) is a wishful thinking and would probably result in old modules that still work not being resilient. So I think 1) is the way to go. But since that doesnt have to happen here in this PR it can be done seperatly after this gets merged.

Comments?

[sdetweil]

yes, 2 is wonderful idea, but we have so many modules that are no longer maintained, this will never happen

so it must be option 1.

I personally hate that we have a fatal config error but continue to run with something else

[khassel]

from my side this can be merged as it doesn't change the current state.

For a future PR: option 1 ...

[rejas]

Created a ticket for the error handling improvement: #2944