Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions src/Microsoft.FeatureManagement/FeatureManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -355,14 +355,6 @@ private async ValueTask<EvaluationEvent> EvaluateFeature<TContext>(string featur
}
}

if (_sessionManagers != null)
{
foreach (ISessionManager sessionManager in _sessionManagers)
{
await sessionManager.SetAsync(evaluationEvent.FeatureDefinition.Name, evaluationEvent.Enabled).ConfigureAwait(false);
}
}

// Only add an activity event if telemetry is enabled for the feature and the activity is valid
if (telemetryEnabled &&
Activity.Current != null &&
Expand All @@ -371,6 +363,28 @@ private async ValueTask<EvaluationEvent> EvaluateFeature<TContext>(string featur
FeatureEvaluationTelemetry.Publish(evaluationEvent, Logger);
}
}
else if (_sessionManagers != null)
{
foreach (ISessionManager sessionManager in _sessionManagers)
{
bool? readSessionResult = await sessionManager.GetAsync(feature).ConfigureAwait(false);

if (readSessionResult.HasValue)
{
evaluationEvent.Enabled = readSessionResult.Value;

break;
}
}
}
Copy link

@apetukhov apetukhov Feb 16, 2026

Choose a reason for hiding this comment

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

I also think that if there is no value in the session manager, then it should be written to what the feature manager returns, i.e. the default value.

foreach (ISessionManager sessionManager in _sessionManagers)
{
await sessionManager.SetAsync(feature, enabled).ConfigureAwait(false);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

The current logic is that if feature definition is not null, then feature manager will do the feature evaluation and the result will be set to session manager.

Did I miss something?

Choose a reason for hiding this comment

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

It doesn't matter whether a feature definition exists; if the feature manager returns a result and doesn't throw an exception, the same result must be set in the session manager.

Test case

  1. Check the boolean feature x in the feature manager.
  2. There is no definition for feature x.
  3. There is no previously saved result in the session manager.
  4. The default result "false" stored in the session manager
  5. The feature manager returns the default result "false"

Step 4 is currently missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't matter whether a feature definition exists; if the feature manager returns a result and doesn't throw an exception, the same result must be set in the session manager.

I believe this is the current behavior (when feature definition is not null).

We introduced the bug in v4. The bug is when feature definition is null, the result won't be set to session manage, I explained it in this comment

Choose a reason for hiding this comment

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

[Fact]
        public async Task SetResultInSessionManagerWhenFeatureDefinitionIsNull()
        {
            IConfiguration config = new ConfigurationBuilder().AddJsonFile("appsettings.json").Build();

            var services = new ServiceCollection();

            ISessionManager sessionManager = new TestSessionManager();

            services
                .AddSingleton(config)
                .AddSingleton<ISessionManager>(sessionManager)
                .AddFeatureManagement();

            ServiceProvider serviceProvider = services.BuildServiceProvider();

            IFeatureManager featureManager = serviceProvider.GetRequiredService<IFeatureManager>();

           // session manager is clean here
            var result = await featureManager.IsEnabledAsync("UnexistedFeature");

           // session manager should store the result after evaluation
             Assert.AreEqual(result, await sessionManager.GetAsync("UnexistedFeature"));
        }

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Updated. Thanks!


if (_sessionManagers != null)
{
foreach (ISessionManager sessionManager in _sessionManagers)
{
await sessionManager.SetAsync(feature, evaluationEvent.Enabled).ConfigureAwait(false);
}
}

return evaluationEvent;
}
Expand Down
55 changes: 55 additions & 0 deletions tests/Tests.FeatureManagement/FeatureManagementTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,61 @@ public async Task ThrowsForMissingFeatures()
featureManager.IsEnabledAsync("NonExistentFeature"));
}

[Fact]
public async Task SessionManagerQueriedWhenFeatureDefinitionIsNull()
{
IConfiguration config = new ConfigurationBuilder().AddJsonFile("appsettings.json").Build();

var services = new ServiceCollection();

ISessionManager sessionManager = new TestSessionManager();

await sessionManager.SetAsync("UnexistedFeature", true);

services
.AddSingleton(config)
.AddSingleton<ISessionManager>(sessionManager)
.AddFeatureManagement();

ServiceProvider serviceProvider = services.BuildServiceProvider();

IFeatureManager featureManager = serviceProvider.GetRequiredService<IFeatureManager>();

// Feature doesn't exist in configuration, but should return true from session
Assert.True(await featureManager.IsEnabledAsync("UnexistedFeature"));

// Set the feature to false in session
await sessionManager.SetAsync("UnexistedFeature", false);

// Should return false from session
Assert.False(await featureManager.IsEnabledAsync("UnexistedFeature"));
}

[Fact]
public async Task SetResultInSessionManagerWhenFeatureDefinitionIsNull()
{
IConfiguration config = new ConfigurationBuilder().AddJsonFile("appsettings.json").Build();

var services = new ServiceCollection();

ISessionManager sessionManager = new TestSessionManager();

services
.AddSingleton(config)
.AddSingleton<ISessionManager>(sessionManager)
.AddFeatureManagement();

ServiceProvider serviceProvider = services.BuildServiceProvider();

IFeatureManager featureManager = serviceProvider.GetRequiredService<IFeatureManager>();

// session manager is clean here
var result = await featureManager.IsEnabledAsync("UnexistedFeature");

// session manager should store the result after evaluation
Assert.Equal(result, await sessionManager.GetAsync("UnexistedFeature"));
}

[Fact]
public async Task ThreadSafeSnapshot()
{
Expand Down
31 changes: 31 additions & 0 deletions tests/Tests.FeatureManagement/TestSessionManager.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
//
using Microsoft.FeatureManagement;
using System.Collections.Concurrent;
using System.Threading.Tasks;

namespace Tests.FeatureManagement
{
class TestSessionManager : ISessionManager
{
private readonly ConcurrentDictionary<string, bool> _session = new ConcurrentDictionary<string, bool>();

public Task SetAsync(string featureName, bool enabled)
{
_session[featureName] = enabled;

return Task.CompletedTask;
}

public Task<bool?> GetAsync(string featureName)
{
if (_session.TryGetValue(featureName, out bool enabled))
{
return Task.FromResult<bool?>(enabled);
}

return Task.FromResult<bool?>(null);
}
}
}
Loading