fix(angular): stop auth actor when destroying `AuthenticatorService` #6333

QuentinFchx · 2025-02-03T20:42:21Z

Description of changes

The xstate machine started during AuthenticatorService initialization was not properly closed.
When starting a machine, it adds a bunch of listeners and register the actor in a global registry (XState interpreter documentation and source on the v4 branch).
Started machine (aka actors) have to be stopped using .stop(). Otherwise it seems the created subscriptions and registrations can leak when used in a long-lived running session such as server-side rendering.

Issue #6293

Description of how you validated changes

I modified my local copy of the memory-leak reproduction to include the present fix.
I then ran 2 memory snapshots (one after 2 requests, and one after 202 requests) to see if it was making any differences : it did, the leak disappeared.

Additional notes

It seems that the Vue implementation has a similar issue : the actor is not stopped when disposing the useAuth composable (as seen here).
I didn't have the time to validate the issue for Vue-based SSR environments so the fix here only apply to Angular. Feel free to investigate further.

EDIT: the use of useActor seems to stop the actor on teardown, please ignore the above note

Checklist

Have read the Pull Request Guidelines
PR description included
yarn test passes and tests are updated/added
PR title and commit messages follow conventional commit syntax
If this change should result in a version bump, changeset added (This can be done after creating the PR.) This does not apply to changes made to docs, e2e, examples, or other private packages.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

changeset-bot · 2025-02-03T20:42:27Z

🦋 Changeset detected

Latest commit: 768eb80

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@aws-amplify/ui-angular	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

tiffanynwyeung

Looks good to me, thanks for reporting the issue and contributing its fix!

HuiSF · 2025-02-05T18:23:54Z

packages/angular/projects/ui-angular/src/lib/services/authenticator.service.ts

@@ -192,6 +192,7 @@ export class AuthenticatorService implements OnDestroy {
  ngOnDestroy(): void {
    if (this._machineSubscription) this._machineSubscription.unsubscribe();
    if (this._unsubscribeHub) this._unsubscribeHub();
+    this._authService.stop();


nit: should we add a unit test to ensure stop() is always being called?

I've tried to add a test for this case by following what has been done already to test the AuthenticatorService
However :

I've had difficulty installing the test stack locally (node-canvas refuses to compile on my macos env)

I'm not sure what are your guidelines about accessing private members (_authService) in tests, I've seen some (... as any) in the codebase so I assumed it was okay.

In any case, I've updated the link to the xstate project to target the v4 branch + I've introduced the initialized property in the mock which complies with the Interpreter definition for that same v4 branch.

If tests were to fail or are not compliying with your standards, can you please take over from there since I can't setup my env properly?

Thank you in advance

Accessing private members seems an existing pattern in the codebase, we can improve this on our side. I'm good with the current test. Thank you for spending time on this!

I'm good with this as well. We can clean up the typing on our end

…is closed on destruction

calebpollman

Thanks for the contribution @QuentinFchx, great job!

* chore(react): remove direct usage of react-test-renderer and types (#6255) * chore(deps): upgrade next deps to point to ^14.2.15 (#6263) * fix(docs): use correct listLocation attribute names in Storage Browser auth example (#6264) * chore(react-native): update allowed peerDep to >=0.70 (#6266) * chore(react): add support for react 19 (#5826) * chore(storage-browser): bump up package size limit (#6267) * chore(changeset): update react 19 changeset from patch to minor bump (#6269) * chore(react): remove radix-ui upgrades (#6268) * fix(docs): remove typo from liveness detector core docs (#6256) * Version Packages (#6261) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * chore: remove temporary pin to react 18 in react@latest build tests (#6274) * chore: remove temporary use of react 18 in react@latest build tests * chore: update workflow for forced test run * chore: adjusting testing strategy, modify publish workflow instead of reusable test * chore: reverting temporary testing changes * Revert support for React 19 (#6278) * Revert "chore: remove temporary pin to react 18 in react@latest build tests (#6274)" This reverts commit d9ee32d. * Revert "fix(docs): remove typo from liveness detector core docs (#6256)" This reverts commit 9d71dd8. * Revert "chore(react): remove radix-ui upgrades (#6268)" This reverts commit c623990. * Revert "chore(changeset): update react 19 changeset from patch to minor bump (#6269)" This reverts commit 7045aed. * Revert "chore(storage-browser): bump up package size limit (#6267)" This reverts commit a79b8e9. * Revert "chore(react): add support for react 19 (#5826)" This reverts commit 954e9be. * chore: add changeset * Version Packages (#6281) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * chore(deps): bump nextjs version (#6284) * chore: add v0.76 and latest to React Native build system tests (#6285) * chore: add react native v0.76 build system tests to CI * chore: add react native 'latest' to build system tests * fix(github-actions): bump upload-artifact to v4 (#6289) * feat(i18n): add Chinese translations for password fields in zh.ts (#6125) Co-authored-by: Caleb Pollman <[email protected]> * test(e2e): add storage-browser offline tests (#6206) * test(e2e): add storage-browser offline tests * address feedback * fix EOF * fix EOF * Update packages/e2e/cypress/integration/common/shared.ts Co-authored-by: Caleb Pollman <[email protected]> * address feedback --------- Co-authored-by: Caleb Pollman <[email protected]> * chore(docs): update react web Authenticator hideSignUp example (#6290) * feat(react): reenable react 19 support (#6296) * Revert "Revert support for React 19 (#6278)" This reverts commit 475e4a3. * update radix deps * remove radix namespace sanitization * migrate/cleanup FileUploader component override example * add ExtendedView component for surfacing ReactNode conflicts * Remove extended and overridden react types * chore(build-system-tests): disable react@latest tests (#6297) * Version Packages (#6292) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * chore(ci): add retry delays to build tests & upgrade actions/cache (#6291) * chore: add delay between install retries, swap RN to use install script * chore: upgrade actions/cache to v4.2.0 * chore(build-system-tests): reenable react@latest tests (#6299) * fix(slider): remounted controlled value not updating (#6301) * fix(ui): fix and add missing sv translations (#6288) Co-authored-by: Jordan Van Ness <[email protected]> Co-authored-by: Caleb Pollman <[email protected]> * chore(changeset): add changeset for PR #6301 (#6303) * fix(react-storage): enable default checksum algorithm for create folder action (#6305) * fix: include default checksum alg header in createFolder action * test: update createFolder test spec for checksum alg header * chore: adding changeset * Version Packages (#6304) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * chore(docs): Update Android UI component versions (#6311) * chore(deps-dev): bump vite from 5.2.14 to 5.4.12 (#6307) Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 5.2.14 to 5.4.12. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/v5.4.12/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v5.4.12/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix(storage-browser): export UseView type (#6314) * chore(ui-react-storage): clean up outdated styles file (#6197) Co-authored-by: ashika112 <[email protected]> * chore(react-native): add fed sign in example env (#6318) * chore: add changeset for #6197 (#6322) * Version Packages (#6315) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * fix(docs): fix typo of fetchUserAttributes (#6324) * chore(gh-workflow): apply maintainer response label only to open issues (#6326) * feature(multi-bucket): add multi-bucket support to storage components (#5562) * initial commit to add 'bucket' property to storage components * chore: use StorageBucket type in StorageImagePathProps * remove duplicate StorageBucket type declaration * chore: update aws-amplify version to include multi-bucket support * docs: include references to new 'bucket' prop and its usage * more explicitly clarifying that can be a string in docs example * chore: changing reference of storage manager to file uploader * chore: updating yarn.lock * chore: undoing unnecessary linting changes * chore: moving yarn.lock from main branch parity * chore: updating yarn.lock to main * chore: add missing references to 'bucket' * chore: adding tests and new example app * chore: add end of file line * chore: add changeset * chore: setting more obviously fake bucket name as example * chore: adding link for setting up multi-bucket configuration to docs * chore: removing unnecessary type definitions * chore: removing unnecessary type from Storage Image props * chore: adding bucket as omitted prop to gen1 props * fix(tests): updating test data to fit expected behavior * chore: adjusting prop order, import consolidation, and added description * chore: add FileUploader example app and e2e test --------- Co-authored-by: Caleb Pollman <[email protected]> * chore: remove storage browser table rows while loading (#6183) * fix(angular): stop auth actor when destroying `AuthenticatorService` (#6333) * fix(angular): stop interpreter when destroying AuthenticatorService * chore: add changeset * test(angular): in AuthenticatorService, ensure underlying auth actor is closed on destruction * fix(docs): add callout for Amplify UI component usage in SSR for Angular/Vue (#6339) * fix(docs): add callout about Amplify UI component usage in SSR for Angular/Vue * chore: adjust wording feedback, fix angular heading sizes * chore(docs): add StorageBrowser amplify auth note (#6342) * chore(deps): remove out of date browserslist resolution (#6343) * chore(deps): clear serialize-javascript and esbuild dependabot alerts (#6348) * fix: add parentheses to fix displayed Storage Browser upload status (#6347) * fix: add parentheses to fix displayed upload status * chore: add changeset * chore: add test for progress indicator * chore: update statuses within progress test to accurately reflect progress state * chore: other minor changes to make test data more appropriate * chore(docs): adjust wording and clean up React troubleshooting page (#6345) * chore(docs): adjust wording and clean up React troubleshooting page * address feedback * revert Next.js version change to point to version introducing breaking changes specifically, 13.4 was the change that introduced stable App Router, and it would be better to point this out rather than point to the latest version of 13 for clarity's sake * chore(changeset): add missing changeset for #6183 (#6353) * Version Packages (#6337) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * chore(deps): bump build-system-test deps, clear esbuild dependabots, remove extraneous resolutions (#6351) * chore(react-storage): update access level deprecation message (#6369) * chore: update url in deprecation message * chore: add changeset * chore(gh-action): bump retry backoff in build system tests (#6355) * fix(docs): remove non-visible header rendering in TOC (#6371) * chore: specify react-native-safe-area-context version (#6372) * chore: move intercepts listeners to before request (#6373) * fix: signout bug when offline (#6061) Co-authored-by: Caleb Pollman <[email protected]> * fix: enable customization of displayText for location detail view dataTable headers (#6346) * fix: enable customization of displayText for location detail view dataTable headers * fix: update location detail view test to include displayText * chore: converting default headers to string array, adding unit tests * chore: add changeset for #6061 (#6375) * chore(deps): add esbuild resolution in build-system-tests/package.json (#6376) * Version Packages (#6370) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * chore(react-core): ensure useDataState returns value of last dispatch (#6382) * Create nasty-lemons-agree.md * feat(ui): allow override of resendSignUpCode function call (#6312) Co-authored-by: Caleb Pollman <[email protected]> * fix: Numeric 0 can be set in the property (#6381) Co-authored-by: James Jarvis <[email protected]> * Version Packages (#6385) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * feat(ui-react-storage): allow custom error boundary (#6408) * feat(ui-react-storage): allow custom error boundary * feat: allow functional error boundary and null to disable --------- Co-authored-by: Danny Banks <[email protected]> * chore: update license allowlist (#6411) --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Caleb Pollman <[email protected]> Co-authored-by: Tiffany Yeung <[email protected]> Co-authored-by: Jordan Van Ness <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: BeforeSunset16 <[email protected]> Co-authored-by: Caleb Pollman <[email protected]> Co-authored-by: Ashwin Kumar <[email protected]> Co-authored-by: Danny Banks <[email protected]> Co-authored-by: berg-dee <[email protected]> Co-authored-by: Vincent Tran <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: AllanZhengYP <[email protected]> Co-authored-by: ashika112 <[email protected]> Co-authored-by: Quentin <[email protected]> Co-authored-by: cp <[email protected]> Co-authored-by: Kevin Campbell <[email protected]> Co-authored-by: Kihara, Takuya <[email protected]>

fix(angular): stop interpreter when destroying AuthenticatorService

1947d35

QuentinFchx requested a review from a team as a code owner February 3, 2025 20:42

chore: add changeset

5dd6319

tiffanynwyeung previously approved these changes Feb 5, 2025

View reviewed changes

jordanvn previously approved these changes Feb 5, 2025

View reviewed changes

HuiSF reviewed Feb 5, 2025

View reviewed changes

cshfang previously approved these changes Feb 5, 2025

View reviewed changes

test(angular): in AuthenticatorService, ensure underlying auth actor …

5264795

…is closed on destruction

QuentinFchx dismissed stale reviews from cshfang, jordanvn, and tiffanynwyeung via 5264795 February 6, 2025 09:53

HuiSF approved these changes Feb 7, 2025

View reviewed changes

calebpollman approved these changes Feb 7, 2025

View reviewed changes

jordanvn approved these changes Feb 7, 2025

View reviewed changes

tiffanynwyeung approved these changes Feb 7, 2025

View reviewed changes

ashika112 approved these changes Feb 7, 2025

View reviewed changes

Merge branch 'main' into angular/fix-memory-leak

768eb80

ashika112 enabled auto-merge (squash) February 7, 2025 18:16

tiffanynwyeung disabled auto-merge February 7, 2025 18:40

HuiSF added run-tests Adding this label will trigger tests to run and removed run-tests Adding this label will trigger tests to run labels Feb 7, 2025

calebpollman removed the run-tests Adding this label will trigger tests to run label Feb 7, 2025

HuiSF merged commit 10b7c9e into aws-amplify:main Feb 7, 2025
48 checks passed

github-actions bot mentioned this pull request Feb 7, 2025

Version Packages #6337

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(angular): stop auth actor when destroying `AuthenticatorService` #6333

fix(angular): stop auth actor when destroying `AuthenticatorService` #6333

QuentinFchx commented Feb 3, 2025 •

edited

Loading

changeset-bot bot commented Feb 3, 2025 •

edited

Loading

tiffanynwyeung left a comment

HuiSF Feb 5, 2025

QuentinFchx Feb 6, 2025 •

edited

Loading

HuiSF Feb 7, 2025

calebpollman Feb 7, 2025

calebpollman left a comment

fix(angular): stop auth actor when destroying AuthenticatorService #6333

fix(angular): stop auth actor when destroying AuthenticatorService #6333

Conversation

QuentinFchx commented Feb 3, 2025 • edited Loading

Description of changes

Issue #6293

Description of how you validated changes

Additional notes

Checklist

changeset-bot bot commented Feb 3, 2025 • edited Loading

🦋 Changeset detected

tiffanynwyeung left a comment

Choose a reason for hiding this comment

HuiSF Feb 5, 2025

Choose a reason for hiding this comment

QuentinFchx Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

HuiSF Feb 7, 2025

Choose a reason for hiding this comment

calebpollman Feb 7, 2025

Choose a reason for hiding this comment

calebpollman left a comment

Choose a reason for hiding this comment

fix(angular): stop auth actor when destroying `AuthenticatorService` #6333

fix(angular): stop auth actor when destroying `AuthenticatorService` #6333

QuentinFchx commented Feb 3, 2025 •

edited

Loading

changeset-bot bot commented Feb 3, 2025 •

edited

Loading

QuentinFchx Feb 6, 2025 •

edited

Loading