Skip to content

Conversation

kelly-yinn
Copy link
Contributor

@kelly-yinn kelly-yinn commented Aug 7, 2025

Closes dotnet/runtime#117976

Adds support for keyed HybridCache instances in DI, along with the ability to configure the DistributedCacheServiceKey for the hybrid cache to resolve a keyed IDistributedCache service. This opens up the scenario of having multiple hybrid caches within an application, each potentially using a separate backend cache. Those backend caches could be separate databases or entirely separate services. For example, having a Redis-backed cache alongside a SQL Server-backed cache.

    [Fact]
    public void CanCreateRedisAndSqlServerBackedHybridCaches()
    {
        var services = new ServiceCollection();
        services.AddKeyedSingleton<IDistributedCache, RedisCache>("Redis");

        services.AddKeyedSingleton<IDistributedCache, SqlServerCache>("SqlServer",
            (sp, key) => new SqlServerCache(new SqlServerCacheOptions
            {
                ConnectionString = "test",
                SchemaName = "test",
                TableName = "test"
            }));

        services.AddKeyedHybridCache("HybridWithRedis", options => options.DistributedCacheServiceKey = "Redis");
        services.AddKeyedHybridCache("HybridWithSqlServer", options => options.DistributedCacheServiceKey = "SqlServer");

        using ServiceProvider provider = services.BuildServiceProvider();
        var hybridWithRedis = Assert.IsType<DefaultHybridCache>(provider.GetRequiredKeyedService<HybridCache>("HybridWithRedis"));
        var hybridWithRedisBackend = Assert.IsType<RedisCache>(hybridWithRedis.BackendCache);
        Assert.Same(hybridWithRedisBackend, provider.GetRequiredKeyedService<IDistributedCache>("Redis"));

        var hybridWithSqlServer = Assert.IsType<DefaultHybridCache>(provider.GetRequiredKeyedService<HybridCache>("HybridWithSqlServer"));
        var hybridWithSqlServerBackend = Assert.IsType<SqlServerCache>(hybridWithSqlServer.BackendCache);
        Assert.Same(hybridWithSqlServerBackend, provider.GetRequiredKeyedService<IDistributedCache>("SqlServer"));
    }
Microsoft Reviewers: Open in CodeFlow

@kelly-yinn kelly-yinn requested a review from a team as a code owner August 7, 2025 02:08
@jeffhandley jeffhandley marked this pull request as draft August 11, 2025 05:44
@jeffhandley
Copy link
Member

I've marked this PR as a draft as we need to get [Support detect keyed dependency injected IDistributedCache as backend cache for DefaultHybridCache (#117976)](dotnet/runtime#117976) through API review, and tests would need to be added to the PR as well.

I'm getting a .NET 10 RC1/RC2 bar check for this. I'll keep you posted, @ylt1534.

@kelly-yinn

This comment was marked as resolved.

@kelly-yinn kelly-yinn marked this pull request as ready for review August 13, 2025 02:36
@kelly-yinn
Copy link
Contributor Author

Thanks @jeffhandley for the review. I just noticed that the DefaultHybridCache is designed as internal thus unable to DI is outside the lib. I added a new extension method and add some UTs. I want to test that the keyed instances (options, backend cache, memory cache, hybrid cache) are injected into each other correctly, but that might bring more changes like expose the _backendCache and _localCache properties from DefaultHybridCache. For now I tried to make the PR small to be easy review. thanks.

@kelly-yinn kelly-yinn marked this pull request as draft August 15, 2025 08:30
@jeffhandley jeffhandley changed the title Update DefaultHybridCache constructor to support DI localCache and backendCache Support keyed HybridCache with keyed DistributedCaches and named options Sep 15, 2025
@eerhardt
Copy link
Member

Is this going to be an issue?

https://github.com/dotnet/aspnetcore/blob/6f91a7d7be082aa1246f41a884b182637942d5a4/src/Caching/StackExchangeRedis/src/RedisCacheImpl.cs#L16-L17

    internal override bool IsHybridCacheActive()
        => _services.GetService<HybridCache>() is not null;

this code assumes that if there is an unkeyed HybridCache in the service collection, then it should add the HC library name suffix:

https://github.com/dotnet/aspnetcore/blob/6f91a7d7be082aa1246f41a884b182637942d5a4/src/Caching/StackExchangeRedis/src/RedisCache.cs#L381-L383

@jeffhandley
Copy link
Member

@eerhardt and I discussed the IsHybridCacheActive scenario noted above and concluded it's not a blocker here. The implementation of that logging/telemetry is not compatible with having keyed HybridCache services registered, but it's also producing the incorrect result if an application uses a Redis cache and also uses a HybridCache that isn't backed by Redis. That design issue and bug should be addressed separately and in a way that is compatible with keyed hybrid caches.

@jeffhandley jeffhandley marked this pull request as ready for review September 16, 2025 23:53
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for keyed HybridCache instances in dependency injection, allowing multiple hybrid caches with different backing distributed caches. The main purpose is to enable scenarios where an application needs multiple hybrid caches, each potentially using separate backend caches (e.g., Redis and SQL Server).

  • Introduces new AddKeyedHybridCache extension methods for registering keyed hybrid cache services
  • Adds DistributedCacheServiceKey property to HybridCacheOptions to specify which keyed distributed cache to use
  • Updates DefaultHybridCache constructor to support keyed distributed cache resolution

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
HybridCacheServiceExtensions.cs Adds new extension methods for keyed hybrid cache registration and refactors existing methods
HybridCacheOptions.cs Adds new property for specifying distributed cache service key
DefaultHybridCache.cs Updates constructor to resolve keyed distributed cache services based on options
ServiceConstructionTests.cs Comprehensive test coverage for new keyed hybrid cache functionality
BasicConfig.json Adds test configuration for distributed cache service keys

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@eerhardt
Copy link
Member

image

Why does this show up as a binary file?

@jeffhandley
Copy link
Member

Why does this show up as a binary file?

It seems these ApiChief baseline JSON files, this one in particular, can match linguist's heuristic for long strings of content that cross a threshold for being identified as binary. I confirmed there's no BOM, no strange encoding or \0 characters, or anything else out of the ordinary. I tried adding a .gitattributes entry for *.json linguist-language=JSON to force an override, but that didn't have any effect. Viewing the file outside the diff view correctly shows it as JSON text. 🤷

@jeffhandley jeffhandley merged commit fdcaa2f into dotnet:main Sep 17, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal] Support keyed HybridCache with keyed DistributedCaches
4 participants