Add IConcurrentPolicyRegistry interface/implementation, for PolicyRegistry

[shagilt]

Why IPolicyRegistry does not have TryAdd() method?

I am seeing KeyAlreadyExistiError when I run this code in multi-threaded test. This is due to multiple requests are trying to add policy at the same time - registry.Add(“TimeoutPolicy”,, retryTimeoutPolicy);

            IPolicyRegistry<string> registry = sp.GetRequiredService<IPolicyRegistry<string>>();
            IAsyncPolicy<HttpResponseMessage> policy = null;
            registry.TryGet<IAsyncPolicy<HttpResponseMessage>>(“TimeoutPolicy”, out policy);
            if (policy == null)
            {
                policy = Policy
                    .TimeoutAsync<HttpResponseMessage>(TimeSpan.FromMilliseconds(10000));

                registry.Add(“TimeoutPolicy”,policy);
            }

            return policy;

I fixed this issue by replacing .Add with this code - registry[“TimeoutPolicy”] = policy; What is the recommended way in this scenario?

This could have solved if we have TryAdd method like ConcurrentDictionary. Why IPolicyRegistry does not have TryAdd() method?

[reisenberger]

@shagilt We would be happy to take a PR to add a TryAdd(...) method with the same syntax as ConcurrentDictionary(...)

[shagilt]

@reisenberger Thank you.

[shagilt]

@reisenberger I will have to update new circuit breaker extension method in aspnet/extension repo to use new TryAdd().

[reisenberger]

@shagilt I gave only a brief answer previously because I was about to go on travel. Coming back to this ...

I'm confused about the source of the problem with registry.Add(“TimeoutPolicy”,policy), because the code I previously proposed for this scenario already uses

registry[key] = newPolicy;

to avoid this, and we also used registry[key] = newPolicy; in the code merged to the new HttpClientFactory extension method.

Compared to TryAdd(...), there is a small risk that, in a highly concurrent scenario, some policy instances are manufactured and placed in the registry but then overwritten (due to race condition) by another instance created at the same time. The non-used instances will get garbage-collected.

[shagilt]

@reisenberger yes, this code is an existing code(before new extension method available in HttpClientFactory). I will have to change our code to use the new extension method.

[reisenberger]

@shagilt I think we are done here, so I'll close this soon unless you need anything else.

On the original point of potentially adding TryAdd() to IPolicyRegistry. We'd be happy to take a PR for that, but I might suggest (to be considered) a new IConcurrentPolicyRegistry interface to avoid breaking changes / placing extra burden on existing implementations of IPolicyRegistry. If doing that, IConcurrentPolicyRegistry should probably provide the suite of Try methods that ConcurrentX provides (eg as ConcurrentDictionary), not just TryAdd(...)

[shagilt]

Sounds good. I am busy next few weeks, I will send the PR after that. Let me know if this is not ok.

[reisenberger]

No problem @shagilt . Sounds great! Thank you for everything you are doing around Polly. 💯

[aerotog]

Reviewing PolicyRegistry, it's already a facade for ConcurrentDictionary, so I think adding a new interface dedicated to concurrent methods seems redundant. Especially since the pattern of including Try methods in the Policy interfaces already exists with TryGet being defined in IReadOnlyPolicyRegistry.

ConcurrentDictionary has a number of methods that don't seem useful/relevant to this ticket. Based on the existing methods in PolicyRegistry (including TryGet), I've added:

• TryAdd
• TryRemove

These have been committed. Once I've added tests for these (probably this weekend), I'll open a PR.

[reisenberger]

Thanks @aerotog! 💯

[shagilt]

Thank you @aerotog

[reisenberger]

Delivered by #715 from @aerotog and #724 from @reisenberger

Merged to the v7.2.0 development branch. Will be released when v7.2.0 is released.