fix: exit server after reconfiguring - Githubissues

rollbar / rollbar.js

Error tracking and logging from Javascript to Rollbar

https://docs.rollbar.com/docs/javascript

MIT License

571 stars 213 forks source link

fix: exit server after reconfiguring #1102

Open alecgibson opened 1 year ago

alecgibson commented 1 year ago

Description of the change

According to the docs, exitOnUncaughtException should:

Exit the process after handling an uncaught exception. Requires captureUncaught to also be set.

So with this config:

var rollbar = new Rollbar({
  captureUncaught: true,
  exitOnUncaughtException: true,
  // ...
});

...an uncaught exception should exit the process.

This works correctly, but only if rollbar.configure() is never called again, because:

exitOnUncaughtException was deleted from our options
Calling configure() calls setupUnhandledCapture()...
...which calls handleUncaughtExceptions()...
...which now tries to access options.exitOnUncaughtException
But this option was deleted in Step 1, so when we replace our uncaughtException handler, we now have exitOnUncaught = false

This change:

no longer deletes exitOnUncaughtException from options (it's unclear why this is the only option that is deleted)
directly accesses options.exitOnUncaughtException in the handler, just like options.captureUncaught is accessed directly

Type of change

[x] Bug fix (non-breaking change that fixes an issue)
[ ] New feature (non-breaking change that adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Maintenance
[ ] New release

Development

[x] Lint rules pass locally
[x] The code changed/added as part of this pull request has been covered with tests
[x] All tests related to the changed code pass in development

Code review

[ ] This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
[ ] "Ready for review" label attached to the PR and reviewers assigned
[ ] Issue from task tracker has a link to this pull request
[ ] Changes have been reviewed by at least one other engineer

alecgibson commented 1 year ago

@waltjones any chance of please getting this reviewed? We've still got an ugly patch in our code to work around this issue.