feat(onUnhandledError): configuration point added for unhandled errors #5681
Conversation
- Adds new configuration setting `onUnhandledError`, which defaults to using "hostReportError" behavior.
- Adds tests that ensure it is called appropriately, and that it is always asynchronous.
- Updates internal name of empty observer to be `EMPTY_OBSERVER` throughout and types it to prevent mutations. Reduces overhead by using the `noop` function for its callbacks.
- Errors that occur during subscription setup _after_ the subscription was already closed will no longer log to `console.warn`
BREAKING CHANGE: Errors that occur during setup of an observable subscription after the subscription has emitted an error or completed will now throw in their own call stack. Before it would call `console.warn`. This is potentially breaking in edge cases for node applications, which may be configured to terminate for unhandled exceptions. In the unlikely event this affects you, you can configure the behavior to `console.warn` in the new configuration setting like so: `import { config } from 'rxjs'; config.onUnhandledError = (err) => console.warn(err);`
benlesh
force-pushed
the
feat/config-onUnhandledError
branch
from
19ed152 to
947c5a2
Compare
| expect(results).to.deep.equal([1]); | ||
| }); | ||
|
|
||
| it('should call asynchronously if an error is emitted and not handled by the consumer next callback', (done) => { |
There was a problem hiding this comment.
nitpick: the description is weird; the consumer's next callback cannot handle the error
| /** | ||
| * Thie test is added so people know this behavior is _intentional_. It's part of the contract of observables | ||
| * and, while I'm not sure I like it, it might start surfacing untold numbers of errors, and break | ||
| * node applications if we suddenly changed this to start throwing errors on other jobs for instances | ||
| * where users accidentally called `subscriber.error` twice. Likewise, would we report an error | ||
| * for two calls of `complete`? This is really something a build-time tool like a linter should | ||
| * capture. Not a run time error reporting event. | ||
| */ |
There was a problem hiding this comment.
It's part of the contract of observables
AFAICT, the contract stipulates how the communication between the observable and the observer is to occur. It says nothing about how errors ought to be treated when then communication channel between the observable and the observer has been closed.
IMO, this behaviour really ought to be configurable. Intentionally swallowing errors when a reporting channel is known to be closed is less than ideal. If a config option were there, I'd definitely be enabling it.
This is really something a build-time tool like a linter should capture. Not a run time error reporting event.
FWIW, for all but the most trivial scenarios, this is beyond the capabilities of static analysis.
There was a problem hiding this comment.
I have some ideas about how this could be addressed. I'll create an issue or a PR after I've given it more thought. In case it's unclear, I think the changes in this PR are all good, but I think we need to do more here.
There was a problem hiding this comment.
The only important thing is that we don't break anyone.
There was a problem hiding this comment.
Sure. The change that I'm contemplating is independent of the onUnhandledError added in this PR, so this is good to merge, IMO.
onUnhandledError, which defaults to using "hostReportError" behavior.EMPTY_OBSERVERthroughout and types it to prevent mutations. Reduces overhead by using thenoopfunction for its callbacks.console.warnBREAKING CHANGE: Errors that occur during setup of an observable subscription after the subscription has emitted an error or completed will now throw in their own call stack. Before it would call
console.warn. This is potentially breaking in edge cases for node applications, which may be configured to terminate for unhandled exceptions. In the unlikely event this affects you, you can configure the behavior toconsole.warnin the new configuration setting like so:import { config } from 'rxjs'; config.onUnhandledError = (err) => console.warn(err);