-
Notifications
You must be signed in to change notification settings - Fork 12k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(@angular/ssr): manage unhandled errors in zoneless applications #28405
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
/** | ||
* @license | ||
* Copyright Google LLC All Rights Reserved. | ||
* | ||
* Use of this source code is governed by an MIT-style license that can be | ||
* found in the LICENSE file at https://angular.dev/license | ||
*/ | ||
|
||
/** | ||
* Attaches listeners to the Node.js process to capture and handle unhandled rejections and uncaught exceptions. | ||
* Captured errors are logged to the console. 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 Zone.js. | ||
* | ||
* @remarks | ||
* This function is a no-op if zone.js is available. | ||
* For Zone-based apps, similar functionality is provided by Zone.js itself. See the Zone.js implementation here: | ||
* https://github.com/angular/angular/blob/4a8d0b79001ec09bcd6f2d6b15117aa6aac1932c/packages/zone.js/lib/node/node.ts#L94%7C | ||
* | ||
* @internal | ||
*/ | ||
export function attachNodeGlobalErrorHandlers(): void { | ||
if (typeof Zone !== 'undefined') { | ||
return; | ||
} | ||
|
||
// Ensure that the listeners are registered only once. | ||
// Otherwise, multiple instances may be registered during edit/refresh. | ||
const gThis: typeof globalThis & { ngAttachNodeGlobalErrorHandlersCalled?: boolean } = globalThis; | ||
if (gThis.ngAttachNodeGlobalErrorHandlersCalled) { | ||
return; | ||
} | ||
|
||
gThis.ngAttachNodeGlobalErrorHandlersCalled = true; | ||
|
||
process | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should Angular servers implement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following the discussions we had recently. I re-opened the PR. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/** | ||
* @license | ||
* Copyright Google LLC All Rights Reserved. | ||
* | ||
* Use of this source code is governed by an MIT-style license that can be | ||
* found in the LICENSE file at https://angular.dev/license | ||
*/ | ||
|
||
declare const Zone: unknown | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative, we can export this a public API, and let the users invoke it manually in the
server.ts