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

Ensure that OptionsCache only permits creating a single options instance per name #79639

Merged
merged 2 commits into from
Dec 17, 2022

Conversation

madelson
Copy link
Contributor

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

fix #79529

…nce per name.

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

fix dotnet#79529
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 14, 2022
@ghost
Copy link

ghost commented Dec 14, 2022

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

Issue Details

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

fix #79529

Author: madelson
Assignees: -
Labels:

area-Extensions-Options

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Dec 14, 2022

@madelson the new test is failing on WASM runs.

13:29:00] fail: [FAIL] Microsoft.Extensions.Options.Tests.OptionsMonitorTest.InstantiatesOnlyOneOptionsInstance
[13:29:00] info: Assert.True() Failure
[13:29:00] info: Expected: True
[13:29:00] info: Actual:   False
[13:29:00] info:    at Microsoft.Extensions.Options.Tests.OptionsMonitorTest.InstantiatesOnlyOneOptionsInstance()
[13:29:00] info:    at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, IntPtr* args)
[13:29:00] info:    at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
[13:29:00] info: Finished:    Microsoft.Extensions.Options.Tests.dll

@tarekgh tarekgh added this to the 8.0.0 milestone Dec 14, 2022
@madelson
Copy link
Contributor Author

@tarekgh any initial thoughts as to why this would fail on wasm? I'm not too familiar with aspects of that runtime that might be different. Would you expect all the main components the test uses to work as they do on desktop (threads, Barrier, AutoResetEvent, etc)?

@tarekgh
Copy link
Member

tarekgh commented Dec 15, 2022

@madelson this may be related to https://github.com/WebAssembly/threads. CC @lewing if has more information here. I would suggest excluding the new test from WASM runs.

@lewing
Copy link
Member

lewing commented Dec 15, 2022

The issue is that you can't do a synchronous wait in the regular wasm runtime. You can either exclude the test or rewrite it to as an async test and await the tasks.

@madelson
Copy link
Contributor Author

Thanks @tarekgh @lewing .

This test is trying to replicate a parallel concurrency issue and is using Barrier.SignalAndWait() and AutoResetEvent.Wait() to force the concurrency. Because we're injecting these into sync flows they can't be replaced with async. Furthermore, on a platform that doesn't support true multi-threading the underlying problem should not reproduce anyway.

For these reasons, I think it makes sense to skip the test on wasm. Let me know if you disagree.

@madelson
Copy link
Contributor Author

@tarekgh does the build analysis look good now? Only shows a known error which seems unrelated.

Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @madelson! LGTM.

@tarekgh tarekgh merged commit 4dd8a78 into dotnet:main Dec 17, 2022
@tarekgh
Copy link
Member

tarekgh commented Jan 3, 2023

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Jan 3, 2023

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3833598902

@ghost ghost locked as resolved and limited conversation to collaborators Feb 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Options community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in Microsoft.Extensions.Options when IOptionsMonitor<T> created concurrently
3 participants