Skip to content
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

Regression in Microsoft.Extensions.Options when IOptionsMonitor<T> created concurrently #79529

Closed
petedishman opened this issue Dec 12, 2022 · 11 comments · Fixed by #79639
Closed
Assignees
Milestone

Comments

@petedishman
Copy link

Description

v7 has introduced a change in behaviour when the first creation of an IOptionMonitor<T> instance is done many times concurrently.

Given configuration code like this:

                services.AddOptions<TestOptions>("testing")
                    .Configure<DummyService>((options, dummyService) =>
                    {
                        Console.WriteLine("Configuring TestOptions instance");
                    });

In dotnet 6 you could create multiple instances of IOptionsMonitor<TestOptions> concurrently on startup and the configuration callback would only be called once.

In dotnet 7, the same operation results in multiple calls to the configuration callback.

This was originally noticed as configuration of Microsoft.Identity.Web authentication stopped working correctly post upgrade to dotnet 7.

I raised a ticket there (AzureAD/microsoft-identity-web#1995), but it was then narrowed down to be a Microsoft.Extensions.Options problem instead, so raising ticket here as @Tratcher suggested.

Reproduction Steps

https://github.com/petedishman/OptionsTest contains a reproduction of this problem with two identical projects, except one is configured with the v6 versions of the libraries, and the other with v7.

The code sets up configuration as above, and then creates (concurrently) multiple instances of a service that take IOptionsMonitor<TestOptions> as a dependency.

When run against v6 libraries you get this output:

C:\build\examples\OptionsTest\OptionsTest6> dotnet run
Building...
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Development
info: Microsoft.Hosting.Lifetime[0]
      Content root path: C:\build\examples\OptionsTest\OptionsTest6
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Configuring TestOptions instance

When run against v7, you get this:

C:\build\examples\OptionsTest\OptionsTest7> dotnet run
Building...
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Development
info: Microsoft.Hosting.Lifetime[0]
      Content root path: C:\build\examples\OptionsTest\OptionsTest7
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Configuring TestOptions instance
Configuring TestOptions instance
Configuring TestOptions instance
Configuring TestOptions instance
Configuring TestOptions instance

After the initial concurrent access, subsequent calls to inject an IOptionsMonitor<TestOptions> instance correctly don't re-run the configuration callback.

Expected behavior

The configuration callback should only be executed when multiple instances are requested concurrently

Actual behavior

The configuration callback is called multiple times when initialised many times concurrently

Regression?

Yes - regression since v6

#66688 seems likely to be related/the cause

Known Workarounds

Initialising the IOptionsMonitor<T> instance once manually before any possible concurrent access works around the problem.

Configuration

Tested on .net 6 & 7 running on both windows & linux x64. Does not appear to be platform specific.

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 12, 2022
@ghost
Copy link

ghost commented Dec 12, 2022

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

Issue Details

Description

v7 has introduced a change in behaviour when the first creation of an IOptionMonitor<T> instance is done many times concurrently.

Given configuration code like this:

                services.AddOptions<TestOptions>("testing")
                    .Configure<DummyService>((options, dummyService) =>
                    {
                        Console.WriteLine("Configuring TestOptions instance");
                    });

In dotnet 6 you could create multiple instances of IOptionsMonitor<TestOptions> concurrently on startup and the configuration callback would only be called once.

In dotnet 7, the same operation results in multiple calls to the configuration callback.

This was originally noticed as configuration of Microsoft.Identity.Web authentication stopped working correctly post upgrade to dotnet 7.

I raised a ticket there (AzureAD/microsoft-identity-web#1995), but it was then narrowed down to be a Microsoft.Extensions.Options problem instead, so raising ticket here as @Tratcher suggested.

Reproduction Steps

https://github.com/petedishman/OptionsTest contains a reproduction of this problem with two identical projects, except one is configured with the v6 versions of the libraries, and the other with v7.

The code sets up configuration as above, and then creates (concurrently) multiple instances of a service that take IOptionsMonitor<TestOptions> as a dependency.

When run against v6 libraries you get this output:

C:\build\examples\OptionsTest\OptionsTest6> dotnet run
Building...
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Development
info: Microsoft.Hosting.Lifetime[0]
      Content root path: C:\build\examples\OptionsTest\OptionsTest6
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Configuring TestOptions instance

When run against v7, you get this:

C:\build\examples\OptionsTest\OptionsTest7> dotnet run
Building...
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Development
info: Microsoft.Hosting.Lifetime[0]
      Content root path: C:\build\examples\OptionsTest\OptionsTest7
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Creating TestOptions instance
Configuring TestOptions instance
Configuring TestOptions instance
Configuring TestOptions instance
Configuring TestOptions instance
Configuring TestOptions instance

After the initial concurrent access, subsequent calls to inject an IOptionsMonitor<TestOptions> instance correctly don't re-run the configuration callback.

Expected behavior

The configuration callback should only be executed when multiple instances are requested concurrently

Actual behavior

The configuration callback is called multiple times when initialised many times concurrently

Regression?

Yes - regression since v6

#66688 seems likely to be related/the cause

Known Workarounds

Initialising the IOptionsMonitor<T> instance once manually before any possible concurrent access works around the problem.

Configuration

Tested on .net 6 & 7 running on both windows & linux x64. Does not appear to be platform specific.

Other information

No response

Author: petedishman
Assignees: -
Labels:

area-Extensions-Options

Milestone: -

@tarekgh tarekgh added this to the 8.0.0 milestone Dec 13, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Dec 13, 2022
@tarekgh tarekgh self-assigned this Dec 13, 2022
@tarekgh
Copy link
Member

tarekgh commented Dec 13, 2022

CC @madelson @eerhardt as there is a suspect #66688 could be the cause. I'll try to look later early next year :-)

madelson added a commit to madelson/runtime that referenced this issue Dec 14, 2022
…nce per name.

This incurs an extra delegate allocation, but only on instance creation.

fix dotnet#79529
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 14, 2022
@madelson
Copy link
Contributor

@tarekgh I think I've identified the issue. See #79639

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 17, 2022
@Tratcher
Copy link
Member

@tarekgh is this a patch candidate? It's a regression that's causing issues in production.

@layomia
Copy link
Contributor

layomia commented Dec 19, 2022

Re-opening per #79529 (comment). @dotnet/area-extensions-options

@tarekgh
Copy link
Member

tarekgh commented Dec 22, 2022

@petedishman @tratacher could you please elaborate more about the exact problem that happened in the production? I know the callback will be called multiple times, but what exactly the side effect with that that break the production?

@tarekgh tarekgh added the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 22, 2022
@ghost
Copy link

ghost commented Dec 22, 2022

This issue has been marked needs-author-action and may be missing some important information.

@tarekgh tarekgh modified the milestones: 8.0.0, 7.0.x Dec 22, 2022
@Tratcher
Copy link
Member

See AzureAD/microsoft-identity-web#1995 where this caused the options instance to be mis-configured such that OIDC events are called multiple times for the same request. These events aren't expecting that and can fail.

@tarekgh tarekgh removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 22, 2022
@davidfowl
Copy link
Member

Yes we should patch this one.

@tarekgh
Copy link
Member

tarekgh commented Dec 22, 2022

I'll start the patch process early next year when I am back. I already marked the issue with the 7.0 milestone.

@petedishman is there a way to try the fix #79639 in your environment to validate the fix?

github-actions bot pushed a commit that referenced this issue Jan 3, 2023
…nce per name.

This incurs an extra delegate allocation, but only on instance creation.

fix #79529
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 4, 2023
carlossanlop pushed a commit that referenced this issue Jan 4, 2023
… options instance per name (#80150)

* Ensure that OptionsCache only permits creating a single options instance per name.

This incurs an extra delegate allocation, but only on instance creation.

fix #79529

* Ignore test on browser

* Add Servicing elements

Co-authored-by: Michael Adelson <mike.adelson314@gmail.com>
Co-authored-by: Tarek Mahmoud Sayed <tarekms@microsoft.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 4, 2023
@tarekgh
Copy link
Member

tarekgh commented Jan 5, 2023

This has been ported to the 7.0 servicing release branch though the PR #80150.

@tarekgh tarekgh closed this as completed Jan 5, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants