fix(node): Avoid catching domain errors in request handler

[mohd-akram]

According to the Node.js docs, domain errors are equivalent to an uncaught exception and therefore should not really be caught except to do some cleanup and then exit the process. Not doing so causes the process to potentially linger without being able to handle any request. Currently, the behavior is also erroneous because next can end up be called twice. This issue was discovered via fastify/middie#158.

Fixes #1922.

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

[AbhiPrasad]

This really just re-affirms that we should switch to async hooks soon enough, but for now I think we can . I'll wait on the rest of my team members to take a look as well though.

[Lms24]

Hi @mohd-akram thanks for opening this PR! The change LGTM

[abrenneke]

It's a bit too late now but this actually ended up being a breaking change for us causing a P0 because we were accidentally relying on this behavior to catch exceptions... I guess we're this person >.< https://xkcd.com/1172/

[lobsterkatie]

Best XKCD ever. 😄

Sorry you had to deal with the P0, though.

[ljearyscg]

by removing this it is no longer working with express when we have sentryErrorMiddleware that checks to see if we should handle the error. This will now always throw the exception.

[Lms24]

I @ljearyscg I'm not sure I'm following why this change impacted the errorhandler. If shouldHandlerError returns false, the exception should not be captured by Sentry. In both cases, the error is passed on via next(error):

sentry-javascript/packages/node/src/handlers.ts

Lines 241 to 278 in 1d5c610

	if (shouldHandleError(error)) {
	withScope(_scope => {
	// For some reason we need to set the transaction on the scope again
	// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
	const transaction = (res as any).__sentry_transaction as Span;
	if (transaction && _scope.getSpan() === undefined) {
	_scope.setSpan(transaction);
	}
	const client = getCurrentHub().getClient<NodeClient>();
	if (client && isAutoSessionTrackingEnabled(client)) {
	// Check if the `SessionFlusher` is instantiated on the client to go into this branch that marks the
	// `requestSession.status` as `Crashed`, and this check is necessary because the `SessionFlusher` is only
	// instantiated when the the`requestHandler` middleware is initialised, which indicates that we should be
	// running in SessionAggregates mode
	// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
	const isSessionAggregatesMode = (client as any)._sessionFlusher !== undefined;
	if (isSessionAggregatesMode) {
	const requestSession = _scope.getRequestSession();
	// If an error bubbles to the `errorHandler`, then this is an unhandled error, and should be reported as a
	// Crashed session. The `_requestSession.status` is checked to ensure that this error is happening within
	// the bounds of a request, and if so the status is updated
	if (requestSession && requestSession.status !== undefined) {
	requestSession.status = 'crashed';
	}
	}
	}
	const eventId = captureException(error);
	// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
	(res as any).sentry = eventId;
	next(error);
	});
	return;
	}
	next(error);

If that is no longer the case, would you mind opening a new issue with an explanation and a reproduction example? Would be greatly appreciated, thanks!