InvalidOperationException thrown when binding operation underway during configuration reload #1189
Comments
|
Tracing it through, it seems like if the causes this enumerable to be walked: which then will call through one of these two LINQ queries: I guess that somehow a reference to the dictionary property, rather than the actual dictionary instance, is being captured somewhere, and then when the reference changes mid-enumeration the exception is thrown by the enumerator. |
|
I'm not sure if it's the root cause, but while looking into this I found that the EnvironmentVariables Have opened #1202 to change that. |
|
It's possible that this is actually a bug in an internal custom configuration source we use, which I'll look into and check on Monday. However, I can consistently reproduce this exception with this test where [Fact]
public void BindingDoesNotThrowIfReloadedDuringBinding()
{
var dic = new Dictionary<string, string>
{
{"Section:Integer", "-2"},
{"Section:Boolean", "TRUe"},
{"Section:Nested:Integer", "11"},
{"Section:Virtual", "Sup"}
};
var configurationBuilder = new ConfigurationBuilder();
configurationBuilder.AddInMemoryCollection(dic);
configurationBuilder.Add(new CustomConfigurationSource());
var config = configurationBuilder.Build();
using (var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(250)))
{
void ReloadLoop()
{
while (!cts.IsCancellationRequested)
{
config.Reload();
}
}
_ = Task.Run(ReloadLoop);
while (!cts.IsCancellationRequested)
{
config.Get<ComplexOptions>();
}
}
}
private sealed class CustomConfigurationSource : IConfigurationSource
{
public IConfigurationProvider Build(IConfigurationBuilder builder)
{
return new CustomConfigurationProvider();
}
}
private sealed class CustomConfigurationProvider : ConfigurationProvider
{
public override void Load()
{
Data["Section:Virtual"] = Guid.NewGuid().ToString();
}
} |
|
Having looked into this more, I think the root cause is the non-atomic reload of the configuration data in the environment variables provider, which is fixed by #1202. |
|
Assuming this fix is approved, would it be eligible for back-porting to 2.1 and/or 2.2? If so, I'd be happy to raise the appropriate PRs. |
|
The patch bar is quite high, the issue has to affect a significant number of users. Calling Reload seems pretty uncommon, what's your scenario for that? How frequently do you call Reload and how often does it fail? |
|
We have a custom When a value is changed in Consul, we send a signal to the running instance(s) of the application(s) affected signaling that they should reload their configuration to pick up whatever these changes are. The frequency of the changes cannot be predicted as they are made on-demand by our engineers as and when they need to update the configuration of an application. In the last 30 days we have had 41 failed HTTP requests due to this specific exception and call path. |
Swap the configuration providers' data atomically, rather than directly changing the property, so that any enumeration of the dictionary running during the reload operation does not throw an InvalidOperationException due to the collection being modified. Relates to #1189.
|
Fixed in 3.0 via the PR. @muratg for the patch discussion. That's a fairly low error rate, and it's not clear that many people have this reload scenario. However the fix was in the EnvironmentVariable provider which every app uses. |
|
|
@muratg @Tratcher I would say yes for 2.2 patch--the bug is pretty bad; I think it should be patched in current. For 2.1, probably not, but I think we should discuss with shiproom before making a final call. The only reason we might take it is that, since it's a race condition bug, it improves the overall stability of 2.1 and these issues are often hard for customers to report effectively because it can be hard to get them to consistently repro. @muratg Are you going to prepare for shiproom, or shall I? |
|
@ajcvickers Please go ahead. I'm guessing it'll just be a PR for 2.2 branch. |
|
@Tratcher Could you send out a PR porting this change to the release/2.2 branch? I'll then fill in the details for shiproom. |
|
Everything has been merged |
ghost
locked as resolved and limited conversation to collaborators
Describe the bug
If
IConfigurationRoot.Reload()is called and the application is serving requests that require an options object to be bound from configuration on other threads, sometimes anInvalidOperationExceptionis thrown causing HTTP 500 responses while the typed config is being bound. An example stack trace is below.Stack trace
To Reproduce
With the application under load with strongly-typed objects used via
IOptions<T>being resolved by requests, callIConfigurationRoot.Reload()by resolving it through DI. The configuration objects probably need to contain several levels of nested objects to cause the issue.Expected behavior
Reload()is called then configuration should be bound from the new configuration.Additional context
The application this exception has been observed with is running ASP.NET Core 2.2.2.
The text was updated successfully, but these errors were encountered: