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

Address LanguageWorkerOptions null in FunctionMetadataManager #10550

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

jviau
Copy link
Contributor

@jviau jviau commented Oct 21, 2024

Issue describing the changes in this PR

resolves #9859

Pull request checklist

IMPORTANT: Currently, changes must be backported to the in-proc branch to be included in Core Tools and non-Flex deployments.

  • Backporting to the in-proc branch is not required
    • Otherwise: Link to backporting PR
  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • 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
  • My changes do not require diagnostic events changes
    • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
  • I have added all required tests (Unit tests, E2E tests)

Additional information

This PR reverts a portion of #9264 and uses a different approach to solve stale function metadata. This alternative approach also addresses the null-ref from #9859.

Background

#9264 set out to solve function metadata being calculated and cached with a stale set of worker configs. To address this, the PR allowed for worker configs to be explicitly supplied to the GetFunctionMetadata method. When configs were not explicitly supplied, it would get 'fallback' configs from the current service provider. However, that provider is the active host. If this was called too early in the hosts lifetime, the active host would be null (leading to the null-ref).

Examining the issue it is a cache invalidation problem. GetFunctionMetadata will get the current value of LanguageWorkerOptions, then build and cache the function metadata results. That caching is the problem, as language worker options changes over the lifetime of the functions host.

Fix

To address the null ref and after taking a closer look at the root issue, a new approach has been taken. Instead of explicitly supplying the RPC configs (which all 2 instances of that were from the current value of LanguageWorkerOptions), we go back to the original approach of getting the current value of those options. With one major difference: we now subscribe to the OnChange of those options and will invalidate the cache whenever they change. The next call to GetFunctionMetadata will then re-build and cache the function metadata set.

Additional Information

#9264 would get the LanguageWorkerOptions from the job host. This PR now gets them from the script host (parent of job host). These options are identical between the two hosts. PR #10369 even sets out to make the job hosts copy of these options a direct forwarding of the script hosts copy.

@jviau jviau requested a review from a team as a code owner October 21, 2024 18:30
@jviau jviau changed the title Address language options null by consuming from parent services Address LanguageWorkerOptions null in FunctionMetadataManager Oct 21, 2024
@jviau
Copy link
Contributor Author

jviau commented Oct 24, 2024

Was testing this further and I am seeing an actual discrepancy between the worker configs between WebHost and ScriptHost, will need to investigate that further.

@liliankasem liliankasem self-requested a review November 7, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullRef in GetFallbackWorkerConfig
2 participants