db-migrate uncaughtException interferes with app uncaughtException

Hurtak commented 3 years ago

I'm submitting a...

[x] Bug report

Current behavior

I am using db-migrate as a library in my app, but since this library uses catches uncaughtException event and does process.exit(0) I am not able to do my own catch uncaughtException and do necessary cleanup before exiting

Expected behavior

ideally db-migrate does not use uncaughtException at all
or, db-migrate sets uncaughtException to true by default
or, there should at lease be a huge disclaimer in the programmable API section of docs, since this is very nonstandard behavior for libraries and it is very hard to debug.

Environment

db-migrate version: 1.0.0-beta.14

Additional information:
- Node version: 14
- Platform: Windows

Hurtak commented 3 years ago

So I found that there is throwUncatched option (https://db-migrate.readthedocs.io/en/latest/API/programable/#getinstanceismodule-options-callback) that solves this, but I think throw should be the default behavior, not process.exit(0), and error handling should be done on the app layer, not overwritten by library.

wzrdtales commented 3 years ago

well that might be what you think, but you forget that db-migrate is a standalone tool in the first place and the programable API is used most often just in wrappers that don't control too much logic. As you easily found out, it is documented, so I don't see problems here and the behavior will not change, since first of all, this would be a breaking change, so never gonna happen for that small issue and further this behavior is wanted to have a controlled output of the errors. Tooling that control errors themself or integrate more than just wrappers should and can just use this option.

If you think the documentation wasn't clear enough about this quirk, feel free to enhance it and shoot a PR.

Hurtak commented 3 years ago

db-migrate is a standalone tool in the first place and the programable API is used most often just in wrappers that don't control too much logic

Yea I guess that could be part of the problem, CLI tool wrapping node-db-migrate will have probably different set of expectations than application using node-db-migrate as a library, and if I understand correctly, they use the same programatical API currently. Maybe there could be 2, where each would have reasonable defaults for its usage (cli tool vs application library)?

As you easily found out

That's the thing, this was very hard to find out, since my uncaughtException got called, but did not behave as expected (wait for all the timers to finish, then end), because there was another uncaughtException handler in this library. So I ended up doing bunch of comment/uncomment code to figure out what was causing this in the big app. I just did not expect a library to interfere with something like this since I have not seen such behavior from other libraries.

it is documented

It is not, it only says throwUncatched - Throw an error instead of calling process.exit(1)

This implies that db-migrate errors are either thrown or that on db-migrate error it calls process.exit(1)

It does not imply that it catches all of the errors, including the ones in your app that uses the library, and potentially can overwrite app error handling with its process.exit(1)

I tracked it down to register-events.js

function registerEvents () {
  process.on('uncaughtException', function (err) {
    log.error('uncaughtException');
    log.error(err.stack || err);
    process.exit(1);
  });

  process.on('unhandledRejection', function (reason) {
    log.error('unhandledRejection');
    log.error(reason.stack || reason);
    process.exit(1);
  });
}

So it basically catches the error, logs the error message, and does process.exit(1) - these steps are identical to what common throw new Error('error message') does, so maybe we could remove this? The unhandledRejection is little trickier, but since Node 15 it behaves the same as regular exceptions (exits the program and logs the stack trace by default)

db-migrate / node-db-migrate