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

Moves cache busting and handles null #438

Merged
merged 3 commits into from
Apr 30, 2024

Conversation

rossgrambo
Copy link
Contributor

@rossgrambo rossgrambo commented Apr 26, 2024

Why this PR?

A developer had an issue. After refresh they were getting:

Object reference not set to an instance of an object

at Microsoft.FeatureManagement.FeatureManager.<GetFeatureNamesAsync>d__10.MoveNext()

After trying to recreate the issue, I found one way. The steps are as follows:

  1. Call FeatureManager.GetFeatureNamesAsync().
  2. Iterate the first item in the AsyncEnumerable. ( This means, ConfigurationFeatureDefinitionProvider.GetAllFeatureDefinitionsAsync will iterate once, meaning it will grab a list of all IConfigurationSections defined within FeatureManagement, but only iterate the first one )
  3. Remove a flag from the Configuration
  4. Refresh configuration
  5. Call IsEnabledAsync("RemovedFlag") or iterate through a full GetFeatureNamesAsync. This fills the cache with "null".
  6. Continue iterating the first AsyncEnumerable.

In short- the cache gets busted when the refresh happens, but the old AsyncEnumerable is not updated. It expects the IConfigurationSection it saw before iterating to still exist.

Visible Changes

This fix is in two places-

  1. The AsyncEnumerable in ConfigurationFeatureDefinitionProvider will check for null iteration- since iterations can be as slow as the developer wants. If the same situation happens- there might be a null in the cache, but we won't return it.

@rossgrambo
Copy link
Contributor Author

Updated to use a null check instead of cache busting.

@rossgrambo rossgrambo merged commit 04c2ba2 into main Apr 30, 2024
2 checks passed
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.

3 participants