fix(@angular/ssr): manage unhandled errors in zoneless applications #28405
Conversation
angular-robot
bot
added
detected: feature
alan-agius4
added
action: review
alan-agius4
changed the title
alan-agius4
changed the title
attachNodeErrorHandlers to manage unhandled errors
alan-agius4
force-pushed
the
handler-errors-node.js
branch
from
79df9ba to
f922b3d
Compare
alan-agius4
added
action: cleanup
alan-agius4
force-pushed
the
handler-errors-node.js
branch
from
f922b3d to
95afc9c
Compare
alan-agius4
changed the title
attachNodeErrorHandlers to manage unhandled errorsattachNodeGlobalErrorHandlers to manage unhandled errors
alan-agius4
force-pushed
the
handler-errors-node.js
branch
2 times, most recently
from
623f923 to
c49f72b
Compare
alan-agius4
removed
the
action: cleanup
alan-agius4
changed the title
attachNodeGlobalErrorHandlers to manage unhandled errors
alan-agius4
added
the
action: review
alan-agius4
force-pushed
the
handler-errors-node.js
branch
from
c49f72b to
905df8f
Compare
alan-agius4
force-pushed
the
handler-errors-node.js
branch
3 times, most recently
from
53536c8 to
81f7fc6
Compare
| // eslint-disable-next-line no-console | ||
| .on('unhandledRejection', (error) => console.error('unhandledRejection', error)) | ||
| // eslint-disable-next-line no-console | ||
| .on('uncaughtException', (error) => console.error('uncaughtException', error)); |
There was a problem hiding this comment.
It's worth noting that this leaves the process in an undefined state which may cause resource leaks and other unexpected behavior. See also: https://nodejs.org/api/process.html#warning-using-uncaughtexception-correctly
Could this behavior be made optional and come with "buyer beware"?
There was a problem hiding this comment.
+1 - discussions with @alxhub and @pkozlowski-opensource before going on leave were around not doing this and needing the framework to handle application errors instead.
This handler makes a choice about errors coming from more than just the framework/application.
There was a problem hiding this comment.
I totally agree, I wasn’t aware that things change since we last talked.
I definitely agree that the FW should handle errors.
There was a problem hiding this comment.
Agree as well - in general I would favor letting a process crash and restart if an error escapes the individual request scope. At that point, you can't really guarantee it can safely continue processing requests.
I wonder if there's potential for some kind of graceful shutdown though, where an uncaught error doesn't terminate the process immediately, but stops accepting new requests and gives some timeout for existing ones to shut down.
There was a problem hiding this comment.
Happy to chat more about options here. Unfortunately, there is (or at least used to be) no easy way to stop accepting new requests (vs. accepting new connections). IIRC the best strategy was to check on the end of requests if there's other concurrent requests on the same connection and using that to separate connections to proxies/load balancers.
There was a problem hiding this comment.
Should Angular servers implement /healthz? :D
There was a problem hiding this comment.
It's a really interesting open question because it says a lot about what this server is. Actually curious about what (if anything) other backend-for-frontend-in-a-box solutions do for this. Wouldn't be surprised if the answer is "not much" since it doesn't matter for most apps if a couple of concurrent requests end up with preventable 500s when something goes wrong..?
There was a problem hiding this comment.
Following the discussions we had recently. I re-opened the PR.
alan-agius4
force-pushed
the
handler-errors-node.js
branch
from
81f7fc6 to
44d8d17
Compare
alan-agius4
requested review from
jkrems,
atscott and
alxhub
and removed request for
pkozlowski-opensource and
alxhub
alan-agius4
force-pushed
the
handler-errors-node.js
branch
from
44d8d17 to
62bf1a8
Compare
alan-agius4
force-pushed
the
handler-errors-node.js
branch
from
62bf1a8 to
e92c432
Compare
| @@ -22,6 +23,10 @@ import { createWebRequestFromNodeRequest } from './request'; | |||
| export class AngularNodeAppEngine { | |||
| private readonly angularAppEngine = new AngularAppEngine(); | |||
|
|
|||
| constructor() { | |||
| attachNodeGlobalErrorHandlers(); | |||
There was a problem hiding this comment.
As an alternative, we can export this a public API, and let the users invoke it manually in the server.ts
alan-agius4
added
action: merge
alan-agius4
force-pushed
the
handler-errors-node.js
branch
from
e92c432 to
72d3485
Compare
Implement the `attachNodeGlobalErrorHandlers` function to handle 'unhandledRejection' and 'uncaughtException' events in Node.js. This function logs errors to the console, preventing unhandled errors from crashing the server. It is particularly useful for zoneless apps, ensuring error handling without relying on zones. Closes angular/angular#58123
alan-agius4
force-pushed
the
handler-errors-node.js
branch
from
72d3485 to
b9efb62
Compare
alan-agius4
added
action: merge
Implement the
attachNodeGlobalErrorHandlersfunction to handle 'unhandledRejection' and 'uncaughtException' events in Node.js. This function logs errors to the console, preventing unhandled errors from crashing the server. It is particularly useful for zoneless apps, ensuring error handling without relying on zones.Closes angular/angular#58123