Make getting lazy properties on ServiceConfig threadsafe #1752
Conversation
JamesNK
requested review from
captainsafia,
BrennanConroy,
brunolins16 and
halter73
| if (_value != null && innerValue == null) | ||
| { | ||
| // Set newly created value | ||
| SetValue(inner, _value); |
There was a problem hiding this comment.
Should the SetValue function also lock since it's public and called by other code?
There was a problem hiding this comment.
ConfigProperty and IConfigValue are both internal so I think it's okay. If this was ever exposed anywhere, locking on this would be bad. But unless it's being exposed as object, I don't see how anyone would easily get a reference.
There was a problem hiding this comment.
Should the SetValue function also lock since it's public and called by other code?
The goal is to make the type thread-safe for reads, e.g. multiple threads can do this without error:
if (serviceConfig.LoadBalancingConfigs.Count > 0)
{
// ....
}If someone calls SetValue value by setting a property on another thread, e.g. serviceConfig.LoadBalancingConfigs = new List<LoadBalancingConfig>() then an error is fine.
| if (_value == null) | ||
| { | ||
| var innerValue = inner.GetValue<TInner>(_key); | ||
| _value = _valueFactory(innerValue); |
There was a problem hiding this comment.
I probably should have asked this before approving, but can _valueFactory call into user code? Or inner.GetValue/SetValue? This reminds me a little bit of the locks I took in ConfigurationManager that caused dotnet/runtime#61747.
Co-authored-by: Brennan <brecon@microsoft.com>
Fixes #1713