Address graceful handling of errors in bedrock events on startup

[mattcollier]

There are multiple open issues that are directly or indirectly related to this topic. Here's some code that demonstrates the problem so we can talk about what we would like to happen in these cases and where the error handling should occur.

The issue of the day for me is that we build MongoDB indexes inside of Bedrock event handlers. If an error occurs during indexing, the error is not properly surfaced and the server continues to operate when it should have failed to start IMO.

Here are my thoughts on what the solution looks like:

• Top level applications should NOT be responsible for catching errors thrown by bedrock.start. We should continue to be able to simply do bedrock.start(); in top level applications as we are already doing now.
• Internally, bedrock should:
  • Properly log the error using the logging system.
  • Shutdown as gracefully as possible. process.exit(1); would be fine for an immediate solution. In the fullness of time, Initiate whatever bedrock.exit process may be created.

const bedrock = require('bedrock');
const delay = require('delay');

bedrock.events.on('bedrock.init', async () => {
  (async () => {
    while (true) {
      console.log('BEDROCK IS STILL RUNNING');
      await delay(1000);
    }
  })();
});

bedrock.events.on('bedrock.ready', async () => {
  throw new Error('ERROR HERE');
});

// without this catch the error in the event is an uncaught promise rejection
// a catch is typically not included in top level applications

// if the catch handler does not shut down the server, the server will
// continue running in a broken state resulting in undefined behavior
bedrock.start().catch(e => console.log('LOG EVENT ERROR', e));

Related issues:
#69
#60
#56
#44

[dlongley]

It's one thing to say that top level applications should not be responsible for handling errors thrown by bedrock.start. However, it's another to say that they can't if they want to. I think it's important to enable top-level applications to handle problems with bedrock.start -- which either means all of them need to do so or we need some kind of option to allow for it.

[mattcollier]

@dlongley what is the use case you have in mind for that functionality. Could we call that a future enhancement to a more expedient solution? Thinking ahead... Wouldn't it be the case that the top level app could register a bedrock.exit handler prior to calling bedrock.start to address any necessary clean-up?

[dlongley]

Every framework that inverts control like that -- or doesn't follow common patterns (e.g., you call an async function and it returns a promise for you to handle) -- increases the likelihood of some kind of awkward / unexpected problem. I'd rather not deviate from common patterns unless we see no other easy way forward. When you do unusual things, expect the unexpected. If we do not deviate from common practice, then we don't have to worry about people coming up with applications that would work with the common composition model but no longer do with bedrock.

This is the sort of thing that created many headaches for us with other frameworks like mocha/protractor/etc -- when we wanted to combine them with another framework of our own (bedrock, for example). I don't want to create similar problems for people (including us) who want to combine bedrock with frameworks X, Y, and Z.

[mattcollier]

Reminder that this issue is unresolved. UnhandledPromiseRejectionWarning occurs when MongoDB encounters indexing issues:

https://gist.github.com/mattcollier/f9ebbdc91f89a9227ff67c7b7ac0272f

[dlongley]

I think this is resolved now. If a promise is rejected that is not handled, bedrock captures it and exits. The app creator also has the option to capture that error via bedrock.start().catch / try { await bedrock.start() } catch (e) { }.