renderModuleFactory does not reject when error is thrown in application #33642

FrozenPandaz · 2019-11-07T04:45:59Z

🐞 bug report

Affected Package

@angular/platform-server

Is this a regression?

No, I tested a v4 and v6 and the behavior is consistent

Description

renderModuleFactory does not reject for errors thrown during rendering.

Examples:

@Component ngOnInit
Router resolve
Probably more

🔬 Minimal Reproduction

https://github.com/FrozenPandaz/angular-bugs/tree/render-module-factory

npm i && npm run prerender

🔥 Exception or Error

 jason@pop-os  ~/projects/temp/temp/angular-bugs   render-module-factory  npm run prerender

> ng8-app@0.0.0 prerender /home/jason/projects/temp/temp/angular-bugs
> ng build && ng run ng8-app:server && node ./main.js

Generating ES5 bundles for differential loading...
ES5 bundle generation complete.

chunk {runtime} runtime-es2015.js, runtime-es2015.js.map (runtime) 6.16 kB [entry] [rendered]
chunk {runtime} runtime-es5.js, runtime-es5.js.map (runtime) 6.16 kB [entry] [rendered]
chunk {styles} styles-es2015.js, styles-es2015.js.map (styles) 10.1 kB [initial] [rendered]
chunk {styles} styles-es5.js, styles-es5.js.map (styles) 14.2 kB [initial] [rendered]
chunk {main} main-es2015.js, main-es2015.js.map (main) 48.2 kB [initial] [rendered]
chunk {main} main-es5.js, main-es5.js.map (main) 55.3 kB [initial] [rendered]
chunk {polyfills-es5} polyfills-es5.js, polyfills-es5.js.map (polyfills-es5) 789 kB [initial] [rendered]
chunk {vendor} vendor-es2015.js, vendor-es2015.js.map (vendor) 3.75 MB [initial] [rendered]
chunk {vendor} vendor-es5.js, vendor-es5.js.map (vendor) 5.21 MB [initial] [rendered]
chunk {polyfills} polyfills-es2015.js, polyfills-es2015.js.map (polyfills) 264 kB [initial] [rendered]
Date: 2019-11-07T04:43:04.373Z - Hash: c69b66b49129c074781b - Time: 13236ms
Hash: 2a979d82dfc31ad8eb0a
Time: 2896ms
Built at: 11/06/2019 11:43:08 PM
      Asset      Size  Chunks             Chunk Names
    main.js    65 KiB    main  [emitted]  main
main.js.map  36.2 KiB    main  [emitted]  main
Entrypoint main = main.js main.js.map
chunk {main} main.js, main.js.map (main) 54.8 KiB [entry] [rendered]
ERROR Error: hi
    at AppComponent.ngOnInit (/home/jason/projects/temp/temp/angular-bugs/dist/ng8-app-server/main.js:233:15)
    at checkAndUpdateDirectiveInline (/home/jason/projects/temp/temp/angular-bugs/node_modules/@angular/core/bundles/core.umd.js:21240:23)
    at checkAndUpdateNodeInline (/home/jason/projects/temp/temp/angular-bugs/node_modules/@angular/core/bundles/core.umd.js:29610:24)
    at checkAndUpdateNode (/home/jason/projects/temp/temp/angular-bugs/node_modules/@angular/core/bundles/core.umd.js:29572:20)
    at prodCheckAndUpdateNode (/home/jason/projects/temp/temp/angular-bugs/node_modules/@angular/core/bundles/core.umd.js:30113:9)
    at Object.updateDirectives (/home/jason/projects/temp/temp/angular-bugs/dist/ng8-app-server/main.js:187:264)
    at Object.updateDirectives (/home/jason/projects/temp/temp/angular-bugs/node_modules/@angular/core/bundles/core.umd.js:29901:76)
    at Object.checkAndUpdateView (/home/jason/projects/temp/temp/angular-bugs/node_modules/@angular/core/bundles/core.umd.js:29554:18)
    at ViewRef_.detectChanges (/home/jason/projects/temp/temp/angular-bugs/node_modules/@angular/core/bundles/core.umd.js:20829:26)
    at ApplicationRef.tick (/home/jason/projects/temp/temp/angular-bugs/node_modules/@angular/core/bundles/core.umd.js:27224:30)
promise resolved
 jason@pop-os  ~/projects/temp/temp/angular-bugs   render-module-factory 

🌍 Your Environment

Angular Version:


 ✘ jason@pop-os  ~/projects/temp/fs-datatable   master  ng v

     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/
    

Angular CLI: 7.3.1
Node: 10.16.3
OS: linux x64
Angular: 7.2.15
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.13.9
@angular-devkit/build-angular     0.13.9
@angular-devkit/build-optimizer   0.13.9
@angular-devkit/build-webpack     0.13.9
@angular-devkit/core              7.3.9
@angular-devkit/schematics        7.3.1
@angular/cdk                      7.3.7
@angular/cli                      7.3.1
@angular/material                 7.3.7
@ngtools/webpack                  7.3.9
@schematics/angular               7.3.1
@schematics/update                0.13.1
rxjs                              6.3.3
typescript                        3.2.2
webpack                           4.29.0

The text was updated successfully, but these errors were encountered:

FrozenPandaz · 2019-11-07T04:46:18Z

cc @vikerman

arturovt · 2019-11-21T17:53:06Z

@FrozenPandaz

I have looked into your reproduction. This exception is caught during the very first change detection. tick is called inside the ApplicationRef.prototype._loadComponent. tick wraps detectChanges in the try-catch statement and uses the default ErrorHandler, that just logs this exception to console.

angular/packages/core/src/application_ref.ts

Lines 682 to 694 in 346e1c1

    
           try { 
        
             this._runningTick = true; 
        
             for (let view of this._views) { 
        
               view.detectChanges(); 
        
             } 
        
             if (this._enforceNoNewChanges) { 
        
               for (let view of this._views) { 
        
                 view.checkNoChanges(); 
        
               } 
        
             } 
        
           } catch (e) { 
        
             // Attention: Don't rethrow as it could cancel subscriptions to Observables! 
        
             this._zone.runOutsideAngular(() => this._exceptionHandler.handleError(e));

You can just override the default ErrorHandler with your custom one on the server and re-throw error except of just logging to the console:

@Injectable()
export class ServerErrorHandler implements ErrorHandler {
  handleError(error: Error): void {
    throw error;
  }
}

@NgModule({
  imports: [AppModule, ServerModule],
  bootstrap: [AppComponent],
  providers: [
    {
      provide: ErrorHandler,
      useClass: ServerErrorHandler
    }
  ]
})
export class AppServerModule {}

And do something like:

try {
  const html = await renderModuleFactory(AppServerModuleNgFactory, {
    document: index
  });
} catch (e) {
  console.log(e);
}

FrozenPandaz · 2019-11-29T22:17:39Z

Thank you so much! Will give that a try. We probably don't want the default to swallow the error though... Do we?

Currently, when there is an error inside a lazy loaded component contructor an unhandled promise rejection is thrown which causes the error handler not to be invoked on the server due to an issue with Zone.js see angular#49930. While this is a workaround for that in general we should not cause unhandled promise rejections and always await and catch promises especially going for going zoneless, this is because Node.js does not wait for pending unhandled promises and these will cause the process to exit. See: nodejs/node#22088 Eventually, the framework will need to be updated to better handle promises in certain listeners for zoneless and avoid unhandled promises Example on potential problematic code. ````ts { provide: APP_BOOTSTRAP_LISTENER, useFactory: () => { Promise.resolve().then(() => {}) }, } ``` This change is also a set closer to address angular#33642

Currently, when there is an error inside a lazy loaded component contructor an unhandled promise rejection is thrown which causes the error handler not to be invoked on the server due to an issue with Zone.js see angular#49930. While this is a workaround for that in general we should not cause unhandled promise rejections and always await and catch promises especially going for going zoneless, this is because Node.js does not wait for pending unhandled promises and these will cause the process to exit. See: nodejs/node#22088 Eventually, the framework will need to be updated to better handle promises in certain listeners for zoneless and avoid unhandled promises Example on potential problematic code. ````ts { provide: APP_BOOTSTRAP_LISTENER, useFactory: () => { Promise.resolve().then(() => {}) }, } ``` This change is also a step closer to address angular#33642

Currently, when there is an error inside a lazy loaded component constructor an unhandled promise rejection is thrown which causes the error handler not to be invoked on the server due to an issue with Zone.js see angular#49930. While this is a workaround for that in general we should not cause unhandled promise rejections and always await and catch promises especially going for going zoneless, this is because Node.js does not wait for pending unhandled promises and these will cause the process to exit. See: nodejs/node#22088 Eventually, the framework will need to be updated to better handle promises in certain listeners for zoneless and avoid unhandled promises Example on potential problematic code. ```ts { provide: APP_BOOTSTRAP_LISTENER, useFactory: () => { Promise.resolve().then(() => {}) }, } ``` This change is also a step closer to address angular#33642

Platonn · 2023-06-23T18:18:36Z

IMHO those are criteria of an ideal solution: (What do you think @alan-agius4?)

in custom Angular app: developers should be able to conveniently decide which types of errors they want to treat as "critical"
in Angular/Universal framework: the framework should propagate those critical errors (with via any medium they like) from the Angular app to the Express app
in custom ExpressJS app: information about those critical errors should be available, returned from the ngExpressEngine in a standard way, familiar to ExpressJS developers, e.g. as a callback (err,html).

Platonn · 2023-06-23T18:26:08Z

I believe there is an architectural/business decision to be made: should errors be propagated immediately when they occur VS. lately only after the app is stable and the rendering finished?

Errors propagated immediately:

Pros:

the client can get an instant response as soon as an error happens. no unnecessary waiting

Cons:

after the error is emitted to Express (and possibly to a client), the rendering is not finished yet(!), meaning that there are still some async tasks. Those tasks can cause side-effects, e.g. make new outgoing http calls, print to logs, write to files(?). It might be confusing that all those things can happen after ngExpressEngine emitted an error. When I'm observing the application activity in the monitoring tools (e.g. logs, OpenTracing), it might be confusing that an "Errored render" is a "zombie" that can still perform side-effects.

Errors propagated lately (only after app is stable):

Pros:

intuitive. "error emitted" means "render is finished"

Cons:

client unnecessarily waits for the render to be completed, even though we already know a critical error occured and we could potentially respond to client's request immediately

I have hard time to choose. My heart says the "late" errors propagation is better, because it's 100% safe and predictable. But my brain tells me it's not as performant as could be.

Just curious, which option you're more fond of, @alan-agius4 ?

Platonn · 2023-06-23T18:31:37Z

By the way, when we agree on the final architectural design, I'm happy to contribute with a PR, if it works for you. The topic of Angular SSR really interests me 😉

Platonn · 2023-12-01T12:35:41Z

Hi @alan-agius4, sorry for bugging you. I'm aware you have lot on your plate.
Is it on your roadmap to consider fixing this issue? Or maybe would you mind if I create a PR?

PS. the duplicate of this issue (#50829) has 23 upvotes 👍

alan-agius4 · 2023-12-01T15:49:15Z

@Platonn, this is definitely something that I want to get too at some point. If you'd like to do a PR feel free to try, although I suspect it is not a trivial change.

Platonn · 2023-12-11T23:29:55Z

Hi @alan-agius4 , thanks for your reply.

Could you help me understand please, which aspects do you perceive as not trivial?

not changing the shape of existing Public API (i.e. avoid breaking changes)?
architecturally deciding on when to propagate errors (immediately VS. only after app isStable)?
... or maybe you meant something else?

If there is anything more I should give a special attention to, please let me know. I'm happy to adapt.

Thanks in advance!

…lizing platform-server In previous versions, if an error occurred within the lifecycle hooks, the promises for `renderModule` and `renderApplication` were not rejected. This could result in serving a "broken" page during Server-Side Rendering (SSR), or the build erroneously being marked as successful during Static Site Generation (SSG). This commit partially resolves issue angular#33642. However, it's important to note that asynchronous lifecycle hooks or hooks containing promises are not currently addressed. This limitation stems from hooks within the framework being floating promises, causing such errors to manifest as unhandled rejections.

…ks when utilizing platform-server In previous versions, if an error occurred within the lifecycle hooks, the promises for `renderModule` and `renderApplication` were not rejected. This could result in serving a "broken" page during Server-Side Rendering (SSR), or the build erroneously being marked as successful during Static Site Generation (SSG). Fixes angular#33642

…izing platform-server In previous versions, if an error occurred within the lifecycle hooks, the promises for `renderModule` and `renderApplication` were not rejected. This could result in serving a "broken" page during Server-Side Rendering (SSR), or the build erroneously being marked as successful during Static Site Generation (SSG). Fixes angular#33642

AndrewKushnir added the area: server Issues related to server-side rendering label Nov 7, 2019

ngbot bot added this to the needsTriage milestone Nov 7, 2019

vikerman added effort2: days freq2: medium severity3: broken type: bug/fix labels Nov 19, 2019

ngbot bot modified the milestones: needsTriage, Backlog Nov 19, 2019

jelbourn added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed severity3: broken labels Oct 1, 2020

AndrewKushnir added P2 The issue is important to a large percentage of users, with a workaround and removed P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Apr 18, 2023

alan-agius4 mentioned this issue Apr 20, 2023

fix(router): handle constructor errors during initial navigation #49953

Closed

alan-agius4 mentioned this issue Apr 20, 2023

Prerender should exit with non 0 status code on Error #49954

Closed

alan-agius4 self-assigned this Apr 20, 2023

alan-agius4 mentioned this issue May 22, 2023

Error handling for prerender angular/universal#1371

Closed

alan-agius4 mentioned this issue Jun 23, 2023

Propagate asynchronous errors during server-side rendering to a client #50829

Closed

alan-agius4 mentioned this issue Dec 12, 2023

Warn when SSR text file routes don't match the configured routes #53522

Closed

alan-agius4 linked a pull request May 14, 2024 that will close this issue

fix(platform-server): propagate errors from lifecycle hooks when utilizing platform-server #55787

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

renderModuleFactory does not reject when error is thrown in application #33642

renderModuleFactory does not reject when error is thrown in application #33642

FrozenPandaz commented Nov 7, 2019

FrozenPandaz commented Nov 7, 2019

arturovt commented Nov 21, 2019 •

edited

FrozenPandaz commented Nov 29, 2019

Platonn commented Jun 23, 2023

Platonn commented Jun 23, 2023 •

edited

Platonn commented Jun 23, 2023 •

edited

Platonn commented Dec 1, 2023 •

edited

alan-agius4 commented Dec 1, 2023

Platonn commented Dec 11, 2023 •

edited

renderModuleFactory does not reject when error is thrown in application #33642

renderModuleFactory does not reject when error is thrown in application #33642

Comments

FrozenPandaz commented Nov 7, 2019

🐞 bug report

Affected Package

Is this a regression?

Description

🔬 Minimal Reproduction

🔥 Exception or Error

🌍 Your Environment

FrozenPandaz commented Nov 7, 2019

arturovt commented Nov 21, 2019 • edited

FrozenPandaz commented Nov 29, 2019

Platonn commented Jun 23, 2023

Platonn commented Jun 23, 2023 • edited

Errors propagated immediately:

Pros:

Cons:

Errors propagated lately (only after app is stable):

Pros:

Cons:

Platonn commented Jun 23, 2023 • edited

Platonn commented Dec 1, 2023 • edited

alan-agius4 commented Dec 1, 2023

Platonn commented Dec 11, 2023 • edited

arturovt commented Nov 21, 2019 •

edited

Platonn commented Jun 23, 2023 •

edited

Platonn commented Jun 23, 2023 •

edited

Platonn commented Dec 1, 2023 •

edited

Platonn commented Dec 11, 2023 •

edited