Skip to content
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

Merged
merged 1 commit into from
Apr 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions goldens/public-api/angular/ssr/node/index.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { Type } from '@angular/core';

// @public
export class AngularNodeAppEngine {
constructor();
handle(request: IncomingMessage | Http2ServerRequest, requestContext?: unknown): Promise<Response | null>;
}

Expand Down
5 changes: 5 additions & 0 deletions packages/angular/ssr/node/src/app-engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import { AngularAppEngine } from '@angular/ssr';
import type { IncomingMessage } from 'node:http';
import type { Http2ServerRequest } from 'node:http2';
import { attachNodeGlobalErrorHandlers } from './errors';
import { createWebRequestFromNodeRequest } from './request';

/**
Expand All @@ -22,6 +23,10 @@ import { createWebRequestFromNodeRequest } from './request';
export class AngularNodeAppEngine {
private readonly angularAppEngine = new AngularAppEngine();

constructor() {
attachNodeGlobalErrorHandlers();
Copy link
Collaborator Author

@alan-agius4 alan-agius4 Apr 1, 2025

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

}

/**
* Handles an incoming HTTP request by serving prerendered content, performing server-side rendering,
* or delivering a static file for client-side rendered routes based on the `RenderMode` setting.
Expand Down
5 changes: 4 additions & 1 deletion packages/angular/ssr/node/src/common-engine/common-engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { renderApplication, renderModule, ɵSERVER_CONTEXT } from '@angular/plat
import * as fs from 'node:fs';
import { dirname, join, normalize, resolve } from 'node:path';
import { URL } from 'node:url';
import { attachNodeGlobalErrorHandlers } from '../errors';
import { CommonEngineInlineCriticalCssProcessor } from './inline-css-processor';
import {
noopRunMethodAndMeasurePerf,
Expand Down Expand Up @@ -63,7 +64,9 @@ export class CommonEngine {
private readonly inlineCriticalCssProcessor = new CommonEngineInlineCriticalCssProcessor();
private readonly pageIsSSG = new Map<string, boolean>();

constructor(private options?: CommonEngineOptions) {}
constructor(private options?: CommonEngineOptions) {
attachNodeGlobalErrorHandlers();
}

/**
* Render an HTML document for a specific URL with specified
Expand Down
40 changes: 40 additions & 0 deletions packages/angular/ssr/node/src/errors.ts
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));
Copy link
Contributor

Choose a reason for hiding this comment

The 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"?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Angular servers implement /healthz? :D

Copy link
Contributor

Choose a reason for hiding this comment

The 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..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the discussions we had recently. I re-opened the PR.

}
9 changes: 9 additions & 0 deletions packages/angular/ssr/node/src/globals.d.ts
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;