Skip to content

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

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

Closed
craigb opened this issue Mar 14, 2024 · 2 comments

Comments

@craigb
Copy link

craigb commented Mar 14, 2024

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.

@craigb craigb changed the title Expose an async hook so Azure Redis can setup the config right before connection? Expose an async hook so Azure Redis can setup the config asynchronously right before connection? Mar 14, 2024
@NickCraver
Copy link
Collaborator

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: dotnet/aspnetcore#24142

@craigb
Copy link
Author

craigb commented Mar 26, 2024

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

@craigb craigb closed this as completed Mar 26, 2024
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

No branches or pull requests

2 participants