GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat(app): add initializeBrowserOrServerApp for browser & server apps by armando-navarro · Pull Request #3844 · angular/angularfire Conversation
|
| const { releaseOnDeref: _releaseOnDeref, releaseAfterRender, automaticDataCollectionEnabled: _automaticDataCollectionEnabled, ...serverSettings } = settings; | ||
| const timedRelease = releaseAfterRender !== undefined || typeof FinalizationRegistry === 'undefined'; | ||
|
|
||
| // Kept reachable only through the injector's destroy hooks, which Angular empties at destroy, so the SDK can collect it right after the render. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The default mode can leak too. If a teardown hook throws (e.g. a service's ngOnDestroy), R3Injector.destroy exits before it clears _onDestroyHooks, so the injector keeps () => renderHolder. Then if the render also left a zone-wrapped listener on the server app (say a root service calling onAuthStateChanged(auth, ...) without unsubscribing), the server app keeps that injector reachable through the listener closure. The holder is never collected and the app never releases. Reproduced with --expose-gc: a throw on its own frees the app, and only a throw plus a listener leaks it.
So this comment and "The default release does not depend on teardown and is not affected" (#3835, docs/auth.md:246, and the PR body) need rewording, e.g. "is affected only when the render also leaves a listener on the server app".
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Fixed rather than reworded: the object handed to the SDK as releaseOnDeref is now a providedIn: 'root' service (ɵRequestLifetime), so it sits in the injector's records, which the finally of destroy clears whatever threw before it.
A spec reproduces your case (a root service whose ngOnDestroy throws, plus a retained reference to the injector) and the app is still released after a collection. The old closure-held object fails that spec.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| if (timedRelease) { | ||
| // Inside an environment injector DestroyRef is the injector itself, so this runs when the render's injector is destroyed. | ||
| destroyRef.onDestroy(() => scheduleRelease(serverApp, releaseAfterRender ?? 0)); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Any teardown throw skips this, not just a service's ngOnDestroy. I ran it with an ApplicationRef.onDestroy listener that throws: the server app stays in _serverApps. From reading Angular 22's teardown (not run), a component ngOnDestroy throw escapes the same way, through ApplicationRef.ngOnDestroy. That's a common SSR bug, e.g. this.sub.unsubscribe() on a browser-only subscription. Worth saying "any ngOnDestroy or destroy hook" in the caveat.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The caveat in the docs and the PR body now says the release is a DestroyRef.onDestroy callback and that an ngOnDestroy method or an ApplicationRef.onDestroy listener that throws keeps it from running.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| }); | ||
|
|
||
| describe('releasing the server app', () => { | ||
| it('by default hands the SDK its own object to release the app at garbage collection', async () => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Nothing here checks that the default-mode release actually happens after GC. Two mutants pass the full suite:
:154. The server app can then be deleted mid-render.renderHolder into a module-level array. That leaks one app per visitor.The callThrough spy also records the holder as a call argument, which keeps it reachable. A spec that drops the spy and forces a collection (--expose-gc) catches both. Have the spec assert the app is still alive while the render runs, then deleted once it's gone.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Added a spec that takes a WeakRef to the registered ɵRequestLifetime, clears the spy's call record, forces a collection while the render is alive (the app must survive), destroys the render, and forces collections until the object is gone and the app is deleted.
test:node and test:node-esm now pass --expose-gc.
Both of your mutants, redone against the service, fail it: handing the SDK a fresh object instead of the injected service deletes the app mid-render, and pushing the service into a module-level array never releases it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
| }); | ||
|
|
||
| it('shares one app between two renders with the same token and releases it after the last', async () => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This catches a double release at delay 0 but not with a positive releaseAfterRender: making the timer at :130 call release twice still passes. Same spec with e.g. releaseAfterRender: 100, asserting the app survives the first timer?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Added the same two-render spec with releaseAfterRender: 100: after the first render's timer fires the app must still be alive for the second render, then deleted after the second. A timer that releases twice fails it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| }); | ||
| }); | ||
|
|
||
| describe('tokens the SDK rejects', () => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The TypeError half of isTokenParseError has no spec. If the check is narrowed to SyntaxError only, the suite stays green, and a token whose payload is null (h.${Buffer.from('null').toString('base64url')}.s) then fails the render with a 500.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Added a spec with a payload of null, which JSON.parse accepts and whose exp read throws a TypeError. It renders signed out with the warning, and narrowing the check to SyntaxError fails it.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
The object handed to the SDK as releaseOnDeref was kept alive by a DestroyRef.onDestroy closure. R3Injector.destroy runs every service's ngOnDestroy before it empties those closures, so a throw there left the object reachable from the injector, and any listener that kept the injector alive kept the server app alive with it. The object is now a providedIn: 'root' service, which the finally of destroy drops whatever threw before it. The timed mode still releases from DestroyRef.onDestroy and a throw before it still skips that release, as the docs say. New specs on the release path: the default release observed through forced collections, a shared app with a positive releaseAfterRender released once per render, a null JSON payload, the App Check half of the token comparison, and a throwing ngOnDestroy next to a retained injector reference. The test:node scripts pass --expose-gc for the forced collections. The resolve hook picks the exports entry the way Node does, the first key in the map's order that is an active condition or default. Refs angular#3844
The caveat on releaseAfterRender said a throwing ngOnDestroy "stops that teardown". It now says the release is a DestroyRef.onDestroy callback, that Angular runs it after every service's and component's ngOnDestroy, and that an ngOnDestroy method or an ApplicationRef.onDestroy listener that throws keeps it from running. The default release, which does not run during the injector's destroy, is unaffected either way. A new paragraph says to call initializeBrowserOrServerApp in app.config.ts rather than a route's providers: a route's child injector is not destroyed with the request, so a release registered there never runs. Refs angular#3844
|
All five inline items are in the new commit, plus two of the follow-ups:
On the other two:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Re-checked on ae887a0, all good. I mutated the built library for each fix (the old closure holder, a fresh object, a module-level array, a double-release timer, SyntaxError-only, no App Check comparison) and each one fails its new spec, and test:node is 440/0.
Two small questions, neither blocking: was the "Verified outside CI" run repeated after the switch to ɵRequestLifetime? And I think the default mode now also releases a provideFirebaseApp placed in a lazy route's providers, since the lifetime is the root service. If that's right, it might be worth a line in #3835.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…iders Tyler asked on angular#3844 whether the default release, now tied to a root service, releases a server app built in a lazy route's providers. It does: the request's root injector is destroyed, the route's child injector is not, so only a releaseAfterRender release registered there is lost. Checked with a child environment injector that is never destroyed, in both modes. Refs angular#3844
|
On the two questions:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
One call to build the right Firebase app for server rendering
There was an error while loading. Please reload this page.