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

Cache TokenCredential #2845

Merged
merged 5 commits into from
Jun 14, 2024
Merged

Cache TokenCredential #2845

merged 5 commits into from
Jun 14, 2024

Conversation

bachuv
Copy link
Collaborator

@bachuv bachuv commented Jun 7, 2024

This PR caches TokenCredential to resolve a memory issue that has been reported when managed identity is enabled. With these changes, we only create the TokenCredential object once instead of every time that GetStorageAccountDetails() is called. This also means that TokenRenewalState is also created once.

These changes were verified by enabling managed identity locally using these instructions and measuring the memory in VS with and without the changes in this PR.

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
  • My changes do not require any extra work to be leveraged by OutOfProc SDKs
    • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
  • My changes do not change the version of the WebJobs.Extensions.DurableTask package
    • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
  • My changes do not add EventIds to our EventSource logs
    • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.
  • My changes should be added to v3.x branch.
    • Otherwise: This change only applies to Durable Functions v2.x and will not be merged to branch v3.x.

@bachuv bachuv added the P1 Priority 1 label Jun 7, 2024
@bachuv bachuv self-assigned this Jun 7, 2024
Copy link
Contributor

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

Some thoughts. In particular, curious if there's a "disposing" hook where we're supposed to remove elements from cachedTokenCredentials.

@bachuv bachuv merged commit 68d8ec9 into dev Jun 14, 2024
20 checks passed
@bachuv bachuv deleted the vabachu/memory-leak-issue branch June 14, 2024 17:12
@bachuv bachuv mentioned this pull request Jul 10, 2024
bachuv added a commit that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Priority 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants