StackExchange / StackExchange.Redis

General purpose redis client
https://stackexchange.github.io/StackExchange.Redis/
Other
5.84k stars 1.5k forks source link

Expose an async hook so Azure Redis can setup the config asynchronously right before connection? #2672

Closed craigb closed 3 months ago

craigb commented 3 months ago

Looking at this package: https://github.com/Azure/Microsoft.Azure.StackExchangeRedis, it seems there's now a weird async context expected during construction. For apps using DI, this becomes ... wonky. Can we add an async hook so that the above package can acquire its OAUTH token right before connection?

Right now, people may be doing:

builder.Services.AddSingleton(ConnectionMultiplexer.Connect(config["redisConnectionString"]);
...
app = builder.Build();
app.Services.GetRequiredService<ConnectionMultiplexer>();
...

For folks trying to use v2 of the https://github.com/Azure/Microsoft.Azure.StackExchangeRedis package, this becomes something like:

var getConnectionMultiplexerTask = async () =>
{
  var config = ConfigurationOptions.Parse(config["redisConnectionString"]);
  config = await config.ConfigureForAzureWithTokenCredentialAsync(config["principalId"], new DefaultAzureCredential());
  return ConnectionMultiplexer.Connect(config);
};
builder.Services.AddSingleton(getConnectionMultiplexerTask);
builder.Services.AddSingleton(static sp =>
{
  var connectionMultiplexerTask = sp.GetRequiredService<Task<ConnectionMultiplexer>>();
  if (connectionMultiplexerTask.IsCompleted)
  {
    return connectionMultiplexerTask.GetAwaiter().GetResult();
  }
  throw new InvalidOperationException("Oops! don't forget to initialize the config for the connection multiplexer!");
})
...
app = builder.Build();
// initialize the multiplexer in an async context
await app.Services.GetRequiredService<Task<ConnectionMultiplexer>>();

// now the following works, so ConnectionMultiplexer can be injected into other classes.
app.Services.GetRequiredService<ConnectionMultiplexer>();

There's already a precedent for one of these hooks. I'm thinking if we can add a BeforeConnectAsync to the ConfigurationOption, then the Azure configuration can do its initialization in there and will appear synchronous from the perspective of the DI container, and the hook can be executed during connection which should already be async.

NickCraver commented 3 months ago

I think the example is a bit more convoluted than normal here because there's an overload in .Connect() and .ConnectAsync() to allow config modification. The equivalent for V2 is more directly:

var getConnectionMultiplexerTask = async () => await ConnectionMultiplexer.ConnectAsync(config["redisConnectionString"], o =>
    await o.ConfigureForAzureWithTokenCredentialAsync(config["principalId"], new DefaultAzureCredential())
);

The reason we don't expose this is that's an actual async thing, e.g. fetching the MSI token from local or even remote caches, or whatever token source. That token is generally not specific to the SE.Redis client for many scenarios (e.g. managed identity) and we'd want that to blow up outside the connection and all the time taken there attributable and debuggable in that phase, not in a sync-over-async (.Connect() is sync) encapsulation hiding what's going on with a thread block.

I think the real global issue here is: configuring services in .NET isn't allowed to be async, and these things are async. This really needs a more global solution upstream so we're all out of sync-over-async in that context and actually allow the application to progress in startup while we establish connections against async endpoints with relatively high time cost compared to local. That issue is tracked here: https://github.com/dotnet/aspnetcore/issues/24142

craigb commented 3 months ago

Thanks for your reasoning -- I'll go ahead and close this.