Skip to content

[release/6.0] Avoid deadlock with ConfigurationManager #63816

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

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

halter73
Copy link
Member

Backport of #62209 to release/6.0

Customer Impact

The following is from a comment @martincostello who originally reported the issue.

From a customer/production perspective, this is an example of the behaviour we saw when the issue first arose.

image

The application was in a steady state serving requests (each coloured line is a different HTTP endpoint), then where the red arrow is when the application's configuration was reloaded in each of the 3 AWS EC2 instances that were in service at the time in response to a change made in our remote configuration store.

The application then very quickly went into a state of deadlock in each instance, with health checks eventually also deadlocking, leading to our load-balancer marking the instances all as unhealthy and taking them out of service. Traffic then flatlines until new instances come into service to take up the load.

I'm not sure if any others have run into the issue, but @martincostello's app wasn't doing anything that unusual. Calling IConfigurationRoot.Reload() isn't super common but it is something I've seen in other apps. Adding or removing config sources at runtime is a new capability of ConfigurationManager and doing that could have also had similar consequences.

Testing

Regression tests were added.

Risk

Medium.

I'm personally confident in the correctness of the change, but I wish it could have been a more surgical fix. The fix removes a lock that was being taken to avoid disposing IConfigurationProviders while they were in use and instead adds reference counting logic to serve the same purpose without needing to lock while calling into arbitrary IConfigurationProvider implementations.

ConfigurationManager is new in .NET 6 which mitigates the risk somewhat. However, it is used by ASP.NET Core's new WebApplication which is now the default host in all the .NET 6 ASP.NET Core project templates, but I think that's all the more reason to fix this deadlock in .NET 6.

@halter73 halter73 added Servicing-consider Issue for next servicing release review area-Extensions-Configuration labels Jan 14, 2022
@halter73 halter73 requested a review from eerhardt January 14, 2022 22:45
@ghost ghost assigned halter73 Jan 14, 2022
@ghost
Copy link

ghost commented Jan 14, 2022

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #62209 to release/6.0

Customer Impact

The following is from a comment @martincostello who originally reported the issue.

From a customer/production perspective, this is an example of the behaviour we saw when the issue first arose.

image

The application was in a steady state serving requests (each coloured line is a different HTTP endpoint), then where the red arrow is when the application's configuration was reloaded in each of the 3 AWS EC2 instances that were in service at the time in response to a change made in our remote configuration store.

The application then very quickly went into a state of deadlock in each instance, with health checks eventually also deadlocking, leading to our load-balancer marking the instances all as unhealthy and taking them out of service. Traffic then flatlines until new instances come into service to take up the load.

I'm not sure if any others have run into the issue, but @martincostello's app wasn't doing anything that unusual. Calling IConfigurationRoot.Reload() isn't super common but it is something I've seen in other apps. Adding or removing config sources at runtime is a new capability of ConfigurationManager and doing that could have also had similar consequences.

Testing

Regression tests were added.

Risk

Medium.

I'm personally confident in the correctness of the change, but I wish it could have been a more surgical fix. The fix removes a lock that was being taken to avoid disposing IConfigurationProviders while they were in use and instead adds reference counting logic to serve the same purpose without needing to lock while calling into arbitrary IConfigurationProvider implementations.

ConfigurationManager is new in .NET 6 which mitigates the risk somewhat. However, it is used by ASP.NET Core's new WebApplication which is now the default host in all the .NET 6 ASP.NET Core project templates, but I think that's all the more reason to fix this deadlock in .NET 6.

Author: halter73
Assignees: -
Labels:

Servicing-consider, area-Extensions-Configuration

Milestone: -

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 20, 2022
@leecow leecow added this to the 6.0.3 milestone Jan 20, 2022
@maryamariyan maryamariyan self-assigned this Jan 25, 2022
@safern
Copy link
Member

safern commented Feb 7, 2022

Failures are unrelated and fixed but the branch would need to be rebased to get them.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants