sentry-javascript: Node: registering `requestHandler` changes error handling behaviour

Review the documentation: https://docs.sentry.io/
Search for existing issues: https://github.com/getsentry/sentry-javascript/issues
Use the latest release: https://github.com/getsentry/sentry-javascript/releases

Package + Version

@sentry/browser
@sentry/node
raven-js
raven-node (raven for node)
other:

Version:

4.6.3

Description

When I enable Sentry.Handlers.requestHandler() (in Express), asynchronous errors will be:

(expected) reported to Sentry
(unexpected) passed on to my Express error handling middleware

It took me a very long time to understand why an async error inside a request handler would lead to different behaviour in development vs production.

In development the error would cause the process to die/exit (default behaviour in Node), whereas in production the error would be passed on to my error handling middleware, and an error response would be sent. It turned out this was because we were only enabling the request handler in production.

I would like to question whether “passing the error to error handling middleware (by calling next and passing the error )” is really the job of Sentry.Handlers.requestHandler(). Perhaps it should only report the error? After all, that’s the main reason to enable Sentry in an application. This way, conditionally enabling Sentry (e.g. depending on env) would not lead to any other unexpected behaviour changes, such as the one I described above.

Alternatively, if we think it is the right thing to do, we should document it clearly, along with a suggestion for how to achieve consistent behaviour when the request handler is not enabled (e.g. by using domains like how Sentry.Handlers.requestHandler() does it).

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 1
Comments: 22 (17 by maintainers)

Most upvoted comments

Currently, when the domain receives an error, we forward the error to Express’ error handling middleware by calling next with the error as an argument:

https://github.com/getsentry/sentry-javascript/blob/913eafc47c7c9ee3bb481615c5ad4016707ce926/packages/node/src/handlers.ts#L236

I think we should change this to only report the error to Sentry:

local.on('error', error => { Sentry.captureException(error); });

This will prevent the requestHandler from modifying Express’ behaviour when an error occurs (which shouldn’t be a concern for requestHandler), as I described in https://github.com/getsentry/sentry-javascript/issues/1922#issuecomment-466376706.

OliverJAsh on Apr 4, 2019

Hi, @LewisJEllis.

Thanks for the info (and a decent laugh at the tweet). We’re discussing which option we want to go with, and will need to do a little testing before we settle on an approach. But it is back on our radar! I’ll update here once we’ve made a decision.

lobsterkatie on Aug 12, 2022

I just ran into this today. This:

local.on('error', next);

bit should not send the error to next.

You can see here in the old raven-node library that I sent domain errors to the global uncaught exception handler: https://github.com/getsentry/raven-node/blame/master/lib/client.js#L442

but in the raven-node to sentry-javascript port, it was changed to send it to express’s next callback instead: https://github.com/getsentry/sentry-javascript/blame/master/packages/node/src/handlers.ts#L156

It is certainly a mistake to send this to the express next callback - that is the root of the behavior difference observed here.

Ideally sentry’s node SDK should move away from domains entirely (various other issues are open for that discussion #4633 #3660 etc), but for a quicker fix I would send it to a generic global uncaught exception handler like in onuncaughtexception.ts (like what raven-node was doing before), or maybe just capture it like @OliverJAsh suggests:

local.on('error', error => { Sentry.captureException(error); });

Or just remove the local.on('error', <whatever>) event handler entirely, and errors should then naturally find their own way to the global uncaughtException handler (assuming one is in place).

LewisJEllis on Aug 9, 2022