Limit Secret loading parallelism #15492
Conversation
sdk/extensions/Azure.Extensions.AspNetCore.Configuration.Secrets/CHANGELOG.md
Outdated
Show resolved
Hide resolved
| { | ||
| private const int ParallelismLevel = 32; | ||
| private readonly SecretClient _client; | ||
| private readonly SemaphoreSlim _semaphore; |
There was a problem hiding this comment.
Thought: could just use a ConcurrentQueue and avoid the housekeeping here.
There was a problem hiding this comment.
Can you explain, please? I don't know a way to use CQ to limit parallelism.
There was a problem hiding this comment.
As noted below, you have a small pool of threads pull work off a queue. I've written/helped write several pipelines that use that approach (mostly setup engines / chainers). The number of threads in the pool is limited, so worker queues are bounded.
There was a problem hiding this comment.
I see. I tried to avoid having to manage my own thread pool, for now, especially considering that async completions would be executed on the real thread pool anyway.
| { | ||
| internal class ParallelSecretLoader: IDisposable | ||
| { | ||
| private const int ParallelismLevel = 32; |
There was a problem hiding this comment.
Is there any practical chance more than one instance is created? For example, could middleware in the app pipeline fetch it's own? Is that enough of a concern to basically design this class to be static and reentrant? You could, for example, keep queueing secrets to fetch and have a pool of worker limited worker threads dequeueing from it (a tactic I've used in the past on a couple different projects).
There was a problem hiding this comment.
More than one instance of this class? I expect it's possible but not too common as you can add multiple KeyVaults to a configuration builder.
I'm a bit reluctant to create a global chokepoint though as those have their own set of scaling issues.
…ts/CHANGELOG.md Co-authored-by: Heath Stewart <[email protected]>
Fixes: #13011