Make TypeDescriptor thread safe with custom providers by enlarging lock region - #92521

Closed
karakasa wants to merge 13 commits into
dotnet:mainfrom
karakasa:issue-92394
Closed

Make TypeDescriptor thread safe with custom providers by enlarging lock region#92521
karakasa wants to merge 13 commits into
dotnet:mainfrom
karakasa:issue-92394

Conversation

@karakasa

@karakasakarakasa commented Sep 23, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Fix#92394 and add a test.

see comment for the reason of race conditions.

The fix is conservative as it just extends the range of lock statement. If the performance decrease in this PR is acceptable, I would recommend enlarging lock region because it is unlikely to result in bugs.

I did code another aggressive approach (#92548) that minimizes lock use but it seems causing stack overflow in certain conditions so I'm not gonna PR that.

Risk

There would be performance impact due to the extended use of lock.

The bottleneck would be querying many to-be-created TypeDescriptionNodes at the same time - as they are forced to be created one by one - which may decrease startup perf. Otherwise the impact should be minimal because most queries quit at the first if block.

@ghostghost added area-System.ComponentModel community-contribution Indicates that the PR has been added by a community member labels Sep 23, 2023
@ghost

ghost commented Sep 23, 2023

Copy link
Copy Markdown

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

Issue Details

Fix #92394 and add a test.

Author:karakasa
Assignees:-
Labels:

area-System.ComponentModel

Milestone:-

@karakasakarakasa changed the title Make TypeDescriptor.GetProperties thread safeMake TypeDescriptor thread safeSep 23, 2023
@karakasakarakasa changed the title Make TypeDescriptor thread safeMake TypeDescriptor thread safe with custom providersSep 23, 2023
@karakasa
Make TypeDescriptor thread safe with custom providers
a8a259c
Originally a race condition exists in `CheckDefaultProvider` and leads
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92934
@karakasa
Originally a race condition exists in CheckDefaultProvider and leads
e4c4296
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92394
@karakasa
karakasa marked this pull request as ready for review September 23, 2023 10:50
@karakasakarakasa mentioned this pull request Sep 24, 2023
Closed
@karakasakarakasa changed the title Make TypeDescriptor thread safe with custom providersMake TypeDescriptor thread safe with custom providers by enlaring lock regionSep 24, 2023
@karakasakarakasa changed the title Make TypeDescriptor thread safe with custom providers by enlaring lock regionMake TypeDescriptor thread safe with custom providers by enlarging lock regionSep 24, 2023
@steveharter

steveharter commented Sep 27, 2023

Copy link
Copy Markdown
Contributor

This touches the same files as the closed PR https://github.com/dotnet/runtime/pull/85156/files for issue #30024. It was closed since it didn't make sense to force this potentially risky issue late into v8.0. Both issues appear to be the same root cause around using the hashtable in s_defaultProviders, but have different use cases so it makes sense to keep open both issues for now.

Performance here will suffer quite a bit - because of the lock area expanding as well as the use of GetCustomAttributes(). The other PR should be used as a reference implementation to address perf, but there may be other ways to address the issues without the extensive changes there and added use the Environment.CurrentManagedThreadId.

@steveharter
steveharter self-requested a review September 27, 2023 21:21
@karakasa
karakasa marked this pull request as draft September 28, 2023 03:24
@karakasa
karakasa marked this pull request as ready for review September 29, 2023 05:25
@karakasa
karakasa marked this pull request as draft October 20, 2023 01:37
steveharter
steveharter reviewed Oct 20, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/ConcurrentTypeDescriptorTests.cs Outdated
public int OneProperty { get; set; }
}
}
}

@stevehartersteveharterOct 20, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think the tests at https://github.com/dotnet/runtime/pull/85156/files#diff-13f56d055c80679284090c59b45d396ab44378385c1f7ad0b597eec15fa5cf0fR813-R909 add any value here?

FWIW locally I added those tests to this file, and ran them in loop for 200 times are no issues were found.

@karakasakarakasaNov 15, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The other tests are not fundamentally different - but I add them if they produce errors on your computer and they don't use Thread (compared to my tests), which is unsupported on browsers.

Nevertheless, both mine and the other tests failed on my PC. Can the issue be platform-specific?

test log
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyClass), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter))​5ms
Error:
Assert.All() Failure: 1 out of 200 items in the collection did not pass.
[2]: Item: System.ComponentModel.TypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Actual: System.ComponentModel.TypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProvider), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter))​2ms
Error:
Assert.All() Failure: 11 out of 200 items in the collection did not pass.
[11]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[10]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[9]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[8]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[7]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[6]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[5]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[4]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[3]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[2]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[0]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected​25ms
Error:
Fallback type descriptor is used. Possible race condition.
Expected: False
Actual: True
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected() in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1287
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in D:\repos\karakasa\runtime\src\coreclr\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.CoreCLR.cs:line 36
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs:line 57

steveharter
steveharter suggested changes Oct 20, 2023
View reviewed changes

@stevehartersteveharter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some comments in the test file.

@ghostghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 20, 2023
@steveharter

steveharter commented Oct 23, 2023

Copy link
Copy Markdown
Contributor

Note the PR is still marked as Draft.

@karakasa

karakasa commented Oct 25, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Note the PR is still marked as Draft.

I am testing on adding another HashTable to reduce lock. Will mark the PR as ready once I decide whether to add the extra hashtable. Do you have thoughts on this?

I will also check the extra tests, thank you.

public sealed class TypeDescriptor
{
....
private static readonly Hashtable s_defaultProviders = new Hashtable();
private static readonly Hashtable s_defaultProvidersCreated = new Hashtable();
....
private static void CheckDefaultProvider(Type type)
{
bool providerAdded = false;
if (s_defaultProvidersCreated.ContainsKey(type))
{
return;
}
lock (s_internalSyncObject)
{
if (s_defaultProviders.ContainsKey(type))
{
return;
}
object[] attrs = type.GetCustomAttributes(typeof(TypeDescriptionProviderAttribute), false);
for (int idx = attrs.Length - 1; idx >= 0; idx--)
{
....
}
s_defaultProvidersCreated[type] = null;
}
....
}
....
}

@ghostghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 25, 2023
@stevehartersteveharter mentioned this pull request Oct 26, 2023
Closed
karakasaand others added 2 commits November 15, 2023 22:47
@karakasa@Maximys
Added another HashTable to reduce locks.
8a7df35
Moved tests to the main testing file
Adopted tests from dotnet#85156
Co-authored-by: Maximys <mixim33@yandex.ru>
@karakasa
better test names
0e3fc99
@karakasa

karakasa commented Nov 15, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

latest benchmark with an extra hashtable to reduce locks. The perf is on par with the main repo.

BenchmarkDotNet v0.13.8, Windows 10 (10.0.19045.2546/22H2/2022Update)
Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.100-rc.2.23502.2
[Host] : .NET 8.0.0 (8.0.23.47906), X64 RyuJIT AVX2
Job-IOOUIL : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-QTNBWJ : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
MethodToolchainMeanErrorStdDevRatioGen0Gen1Gen2AllocatedAlloc Ratio
MultipleThreadsmain1,987.632 μs14.8732 μs13.9124 μs1.0035.15637.81257.812510.39 KB1.00
MultipleThreadspr1,959.046 μs21.4355 μs20.0508 μs0.9935.15637.81257.812510.39 KB1.00
SingleThreadmain5.042 μs0.0173 μs0.0145 μs1.000.3052--1.92 KB1.00
SingleThreadpr5.061 μs0.0664 μs0.0621 μs1.000.3052--1.92 KB1.00

@karakasa
karakasa marked this pull request as ready for review November 15, 2023 16:10
@karakasa
karakasa marked this pull request as draft November 16, 2023 02:21
@karakasa
Skip one test on browsers
33d5a3e
ConcurrentGetProperties_ReturnsExpected
is skipped on browsers because Thread.Start
is unsupported.
@karakasa
karakasa marked this pull request as ready for review November 16, 2023 03:59
@build-analysisbuild-analysisBot mentioned this pull request Nov 16, 2023
Closed
@ericstjericstj mentioned this pull request Dec 12, 2023
Closed
@ericstj

ericstj commented Dec 12, 2023

Copy link
Copy Markdown
Member

Before seeing this PR I happened to be looking at this problem and made a suggestion in #30024. I think that suggestion could be considered here as a slight alternative to what's been implemented so far.

@stevehartersteveharter mentioned this pull request Jan 11, 2024
Merged
@steveharter

steveharter commented Jan 11, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Replaced by #96846 so we can apply the sentinel pattern mentioned in #30024

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 11, 2024
@karakasa
karakasa deleted the issue-92394 branch May 25, 2024 08:45
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stevehartersteveharterAwaiting requested review from steveharter

Assignees

@stevehartersteveharter

Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TypeDescriptor.GetProperties(object instance) is not thread-safe

3 participants

@karakasa@steveharter@ericstj
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Make TypeDescriptor thread safe with custom providers by enlarging lock region - #92521
Closed
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Closed
Make TypeDescriptor thread safe with custom providers by enlarging lock region#92521
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Conversation
@karakasa
@karakasakarakasa
commented
Sep 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Summary
Fix#92394 and add a test.
see comment for the reason of race conditions.
The fix is conservative as it just extends the range of lock statement. If the performance decrease in this PR is acceptable, I would recommend enlarging lock region because it is unlikely to result in bugs.
I did code another aggressive approach (#92548) that minimizes lock use but it seems causing stack overflow in certain conditions so I'm not gonna PR that.
Risk
There would be performance impact due to the extended use of lock.
The bottleneck would be querying many to-be-created TypeDescriptionNodes at the same time - as they are forced to be created one by one - which may decrease startup perf. Otherwise the impact should be minimal because most queries quit at the first if block.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Sep 23, 2023
@ghost
ghost
commented
Sep 23, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
Fix #92394 and add a test.
Author:karakasa
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@karakasakarakasa
changed the title
Make TypeDescriptor.GetProperties thread safeMake TypeDescriptor thread safeSep 23, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safeMake TypeDescriptor thread safe with custom providersSep 23, 2023
@karakasa
Make TypeDescriptor thread safe with custom providers
a8a259c
Originally a race condition exists in `CheckDefaultProvider` and leads
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92934
@karakasa
Originally a race condition exists in CheckDefaultProvider and leads
e4c4296
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92394
@karakasa
karakasa
marked this pull request as ready for review
September 23, 2023 10:50
@karakasakarakasa
mentioned this pull request
Sep 24, 2023
 Closed

@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providersMake TypeDescriptor thread safe with custom providers by enlaring lock regionSep 24, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providers by enlaring lock regionMake TypeDescriptor thread safe with custom providers by enlarging lock regionSep 24, 2023
@steveharter
steveharter
commented
Sep 27, 2023

Copy link


Copy Markdown

Contributor
This touches the same files as the closed PR https://github.com/dotnet/runtime/pull/85156/files for issue #30024. It was closed since it didn't make sense to force this potentially risky issue late into v8.0. Both issues appear to be the same root cause around using the hashtable in s_defaultProviders, but have different use cases so it makes sense to keep open both issues for now.
Performance here will suffer quite a bit - because of the lock area expanding as well as the use of GetCustomAttributes(). The other PR should be used as a reference implementation to address perf, but there may be other ways to address the issues without the extensive changes there and added use the Environment.CurrentManagedThreadId.
@steveharter
steveharter
self-requested a review
September 27, 2023 21:21
@karakasa
karakasa
marked this pull request as draft
September 28, 2023 03:24
@karakasa
karakasa
marked this pull request as ready for review
September 29, 2023 05:25
@karakasa
karakasa
marked this pull request as draft
October 20, 2023 01:37
steveharter
steveharter
reviewed
Oct 20, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/ConcurrentTypeDescriptorTests.cs
Outdated

 public int OneProperty { get; set; }
 }
 }
}
@stevehartersteveharterOct 20, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Do you think the tests at https://github.com/dotnet/runtime/pull/85156/files#diff-13f56d055c80679284090c59b45d396ab44378385c1f7ad0b597eec15fa5cf0fR813-R909 add any value here?
FWIW locally I added those tests to this file, and ran them in loop for 200 times are no issues were found.
@karakasakarakasaNov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The other tests are not fundamentally different - but I add them if they produce errors on your computer and they don't use Thread (compared to my tests), which is unsupported on browsers.
Nevertheless, both mine and the other tests failed on my PC. Can the issue be platform-specific?
test log
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyClass), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter))​5ms
Error:
Assert.All() Failure: 1 out of 200 items in the collection did not pass.
[2]: Item: System.ComponentModel.TypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Actual: System.ComponentModel.TypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProvider), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter))​2ms
Error:
Assert.All() Failure: 11 out of 200 items in the collection did not pass.
[11]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[10]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[9]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[8]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[7]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[6]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[5]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[4]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[3]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[2]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[0]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected​25ms
Error:
Fallback type descriptor is used. Possible race condition.
Expected: False
Actual: True
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected() in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1287
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in D:\repos\karakasa\runtime\src\coreclr\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.CoreCLR.cs:line 36
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs:line 57

steveharter
steveharter
suggested changes
Oct 20, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left some comments in the test file.
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 20, 2023
@steveharter
steveharter
commented
Oct 23, 2023

Copy link


Copy Markdown

Contributor
Note the PR is still marked as Draft.
@karakasa
karakasa
commented
Oct 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Note the PR is still marked as Draft.
I am testing on adding another HashTable to reduce lock. Will mark the PR as ready once I decide whether to add the extra hashtable. Do you have thoughts on this?
I will also check the extra tests, thank you.
public sealed class TypeDescriptor
{
....
private static readonly Hashtable s_defaultProviders = new Hashtable();
private static readonly Hashtable s_defaultProvidersCreated = new Hashtable();
....
private static void CheckDefaultProvider(Type type)
{
bool providerAdded = false;
if (s_defaultProvidersCreated.ContainsKey(type))
{
return;
}
lock (s_internalSyncObject)
{
if (s_defaultProviders.ContainsKey(type))
{
return;
}
object[] attrs = type.GetCustomAttributes(typeof(TypeDescriptionProviderAttribute), false);
for (int idx = attrs.Length - 1; idx >= 0; idx--)
{
....
}
s_defaultProvidersCreated[type] = null;
}
....
}
....
}

@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 25, 2023
@stevehartersteveharter
mentioned this pull request
Oct 26, 2023

Closed

karakasaand others
added 2 commits
November 15, 2023 22:47
@karakasa@Maximys
Added another HashTable to reduce locks.
8a7df35
Moved tests to the main testing file
Adopted tests from dotnet#85156
Co-authored-by: Maximys <mixim33@yandex.ru>
@karakasa
better test names
0e3fc99
@karakasa
karakasa
commented
Nov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
latest benchmark with an extra hashtable to reduce locks. The perf is on par with the main repo.
BenchmarkDotNet v0.13.8, Windows 10 (10.0.19045.2546/22H2/2022Update)
Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.100-rc.2.23502.2
[Host] : .NET 8.0.0 (8.0.23.47906), X64 RyuJIT AVX2
Job-IOOUIL : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-QTNBWJ : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

MethodToolchainMeanErrorStdDevRatioGen0Gen1Gen2AllocatedAlloc Ratio
MultipleThreadsmain1,987.632 μs14.8732 μs13.9124 μs1.0035.15637.81257.812510.39 KB1.00
MultipleThreadspr1,959.046 μs21.4355 μs20.0508 μs0.9935.15637.81257.812510.39 KB1.00
SingleThreadmain5.042 μs0.0173 μs0.0145 μs1.000.3052--1.92 KB1.00
SingleThreadpr5.061 μs0.0664 μs0.0621 μs1.000.3052--1.92 KB1.00
@karakasa
karakasa
marked this pull request as ready for review
November 15, 2023 16:10
@karakasa
karakasa
marked this pull request as draft
November 16, 2023 02:21
@karakasa
Skip one test on browsers
33d5a3e
ConcurrentGetProperties_ReturnsExpected
is skipped on browsers because Thread.Start
is unsupported.
@karakasa
karakasa
marked this pull request as ready for review
November 16, 2023 03:59
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 16, 2023

Closed

@ericstjericstj
mentioned this pull request
Dec 12, 2023

Closed

@ericstj
ericstj
commented
Dec 12, 2023

Copy link


Copy Markdown

Member
Before seeing this PR I happened to be looking at this problem and made a suggestion in #30024. I think that suggestion could be considered here as a slight alternative to what's been implemented so far.
@stevehartersteveharter
mentioned this pull request
Jan 11, 2024
 Merged

@steveharter
steveharter
commented
Jan 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Replaced by #96846 so we can apply the sentinel pattern mentioned in #30024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 11, 2024
@karakasa
karakasa
deleted the

issue-92394

branch
May 25, 2024 08:45
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stevehartersteveharterAwaiting requested review from steveharter

Assignees

@stevehartersteveharter

Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

TypeDescriptor.GetProperties(object instance) is not thread-safe


3 participants

@karakasa@steveharter@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make TypeDescriptor thread safe with custom providers by enlarging lock region - #92521
Closed
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Closed
Make TypeDescriptor thread safe with custom providers by enlarging lock region#92521
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Conversation
@karakasa
@karakasakarakasa
commented
Sep 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Summary
Fix#92394 and add a test.
see comment for the reason of race conditions.
The fix is conservative as it just extends the range of lock statement. If the performance decrease in this PR is acceptable, I would recommend enlarging lock region because it is unlikely to result in bugs.
I did code another aggressive approach (#92548) that minimizes lock use but it seems causing stack overflow in certain conditions so I'm not gonna PR that.
Risk
There would be performance impact due to the extended use of lock.
The bottleneck would be querying many to-be-created TypeDescriptionNodes at the same time - as they are forced to be created one by one - which may decrease startup perf. Otherwise the impact should be minimal because most queries quit at the first if block.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Sep 23, 2023
@ghost
ghost
commented
Sep 23, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
Fix #92394 and add a test.
Author:karakasa
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@karakasakarakasa
changed the title
Make TypeDescriptor.GetProperties thread safeMake TypeDescriptor thread safeSep 23, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safeMake TypeDescriptor thread safe with custom providersSep 23, 2023
@karakasa
Make TypeDescriptor thread safe with custom providers
a8a259c
Originally a race condition exists in `CheckDefaultProvider` and leads
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92934
@karakasa
Originally a race condition exists in CheckDefaultProvider and leads
e4c4296
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92394
@karakasa
karakasa
marked this pull request as ready for review
September 23, 2023 10:50
@karakasakarakasa
mentioned this pull request
Sep 24, 2023
 Closed

@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providersMake TypeDescriptor thread safe with custom providers by enlaring lock regionSep 24, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providers by enlaring lock regionMake TypeDescriptor thread safe with custom providers by enlarging lock regionSep 24, 2023
@steveharter
steveharter
commented
Sep 27, 2023

Copy link


Copy Markdown

Contributor
This touches the same files as the closed PR https://github.com/dotnet/runtime/pull/85156/files for issue #30024. It was closed since it didn't make sense to force this potentially risky issue late into v8.0. Both issues appear to be the same root cause around using the hashtable in s_defaultProviders, but have different use cases so it makes sense to keep open both issues for now.
Performance here will suffer quite a bit - because of the lock area expanding as well as the use of GetCustomAttributes(). The other PR should be used as a reference implementation to address perf, but there may be other ways to address the issues without the extensive changes there and added use the Environment.CurrentManagedThreadId.
@steveharter
steveharter
self-requested a review
September 27, 2023 21:21
@karakasa
karakasa
marked this pull request as draft
September 28, 2023 03:24
@karakasa
karakasa
marked this pull request as ready for review
September 29, 2023 05:25
@karakasa
karakasa
marked this pull request as draft
October 20, 2023 01:37
steveharter
steveharter
reviewed
Oct 20, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/ConcurrentTypeDescriptorTests.cs
Outdated

 public int OneProperty { get; set; }
 }
 }
}
@stevehartersteveharterOct 20, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Do you think the tests at https://github.com/dotnet/runtime/pull/85156/files#diff-13f56d055c80679284090c59b45d396ab44378385c1f7ad0b597eec15fa5cf0fR813-R909 add any value here?
FWIW locally I added those tests to this file, and ran them in loop for 200 times are no issues were found.
@karakasakarakasaNov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The other tests are not fundamentally different - but I add them if they produce errors on your computer and they don't use Thread (compared to my tests), which is unsupported on browsers.
Nevertheless, both mine and the other tests failed on my PC. Can the issue be platform-specific?
test log
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyClass), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter))​5ms
Error:
Assert.All() Failure: 1 out of 200 items in the collection did not pass.
[2]: Item: System.ComponentModel.TypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Actual: System.ComponentModel.TypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProvider), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter))​2ms
Error:
Assert.All() Failure: 11 out of 200 items in the collection did not pass.
[11]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[10]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[9]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[8]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[7]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[6]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[5]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[4]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[3]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[2]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[0]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected​25ms
Error:
Fallback type descriptor is used. Possible race condition.
Expected: False
Actual: True
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected() in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1287
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in D:\repos\karakasa\runtime\src\coreclr\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.CoreCLR.cs:line 36
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs:line 57

steveharter
steveharter
suggested changes
Oct 20, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left some comments in the test file.
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 20, 2023
@steveharter
steveharter
commented
Oct 23, 2023

Copy link


Copy Markdown

Contributor
Note the PR is still marked as Draft.
@karakasa
karakasa
commented
Oct 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Note the PR is still marked as Draft.
I am testing on adding another HashTable to reduce lock. Will mark the PR as ready once I decide whether to add the extra hashtable. Do you have thoughts on this?
I will also check the extra tests, thank you.
public sealed class TypeDescriptor
{
....
private static readonly Hashtable s_defaultProviders = new Hashtable();
private static readonly Hashtable s_defaultProvidersCreated = new Hashtable();
....
private static void CheckDefaultProvider(Type type)
{
bool providerAdded = false;
if (s_defaultProvidersCreated.ContainsKey(type))
{
return;
}
lock (s_internalSyncObject)
{
if (s_defaultProviders.ContainsKey(type))
{
return;
}
object[] attrs = type.GetCustomAttributes(typeof(TypeDescriptionProviderAttribute), false);
for (int idx = attrs.Length - 1; idx >= 0; idx--)
{
....
}
s_defaultProvidersCreated[type] = null;
}
....
}
....
}

@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 25, 2023
@stevehartersteveharter
mentioned this pull request
Oct 26, 2023

Closed

karakasaand others
added 2 commits
November 15, 2023 22:47
@karakasa@Maximys
Added another HashTable to reduce locks.
8a7df35
Moved tests to the main testing file
Adopted tests from dotnet#85156
Co-authored-by: Maximys <mixim33@yandex.ru>
@karakasa
better test names
0e3fc99
@karakasa
karakasa
commented
Nov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
latest benchmark with an extra hashtable to reduce locks. The perf is on par with the main repo.
BenchmarkDotNet v0.13.8, Windows 10 (10.0.19045.2546/22H2/2022Update)
Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.100-rc.2.23502.2
[Host] : .NET 8.0.0 (8.0.23.47906), X64 RyuJIT AVX2
Job-IOOUIL : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-QTNBWJ : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

MethodToolchainMeanErrorStdDevRatioGen0Gen1Gen2AllocatedAlloc Ratio
MultipleThreadsmain1,987.632 μs14.8732 μs13.9124 μs1.0035.15637.81257.812510.39 KB1.00
MultipleThreadspr1,959.046 μs21.4355 μs20.0508 μs0.9935.15637.81257.812510.39 KB1.00
SingleThreadmain5.042 μs0.0173 μs0.0145 μs1.000.3052--1.92 KB1.00
SingleThreadpr5.061 μs0.0664 μs0.0621 μs1.000.3052--1.92 KB1.00
@karakasa
karakasa
marked this pull request as ready for review
November 15, 2023 16:10
@karakasa
karakasa
marked this pull request as draft
November 16, 2023 02:21
@karakasa
Skip one test on browsers
33d5a3e
ConcurrentGetProperties_ReturnsExpected
is skipped on browsers because Thread.Start
is unsupported.
@karakasa
karakasa
marked this pull request as ready for review
November 16, 2023 03:59
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 16, 2023

Closed

@ericstjericstj
mentioned this pull request
Dec 12, 2023

Closed

@ericstj
ericstj
commented
Dec 12, 2023

Copy link


Copy Markdown

Member
Before seeing this PR I happened to be looking at this problem and made a suggestion in #30024. I think that suggestion could be considered here as a slight alternative to what's been implemented so far.
@stevehartersteveharter
mentioned this pull request
Jan 11, 2024
 Merged

@steveharter
steveharter
commented
Jan 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Replaced by #96846 so we can apply the sentinel pattern mentioned in #30024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 11, 2024
@karakasa
karakasa
deleted the

issue-92394

branch
May 25, 2024 08:45
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stevehartersteveharterAwaiting requested review from steveharter

Assignees

@stevehartersteveharter

Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

TypeDescriptor.GetProperties(object instance) is not thread-safe


3 participants

@karakasa@steveharter@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make TypeDescriptor thread safe with custom providers by enlarging lock region - #92521
Closed
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Closed
Make TypeDescriptor thread safe with custom providers by enlarging lock region#92521
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Conversation
@karakasa
@karakasakarakasa
commented
Sep 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Summary
Fix#92394 and add a test.
see comment for the reason of race conditions.
The fix is conservative as it just extends the range of lock statement. If the performance decrease in this PR is acceptable, I would recommend enlarging lock region because it is unlikely to result in bugs.
I did code another aggressive approach (#92548) that minimizes lock use but it seems causing stack overflow in certain conditions so I'm not gonna PR that.
Risk
There would be performance impact due to the extended use of lock.
The bottleneck would be querying many to-be-created TypeDescriptionNodes at the same time - as they are forced to be created one by one - which may decrease startup perf. Otherwise the impact should be minimal because most queries quit at the first if block.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Sep 23, 2023
@ghost
ghost
commented
Sep 23, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
Fix #92394 and add a test.
Author:karakasa
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@karakasakarakasa
changed the title
Make TypeDescriptor.GetProperties thread safeMake TypeDescriptor thread safeSep 23, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safeMake TypeDescriptor thread safe with custom providersSep 23, 2023
@karakasa
Make TypeDescriptor thread safe with custom providers
a8a259c
Originally a race condition exists in `CheckDefaultProvider` and leads
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92934
@karakasa
Originally a race condition exists in CheckDefaultProvider and leads
e4c4296
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92394
@karakasa
karakasa
marked this pull request as ready for review
September 23, 2023 10:50
@karakasakarakasa
mentioned this pull request
Sep 24, 2023
 Closed

@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providersMake TypeDescriptor thread safe with custom providers by enlaring lock regionSep 24, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providers by enlaring lock regionMake TypeDescriptor thread safe with custom providers by enlarging lock regionSep 24, 2023
@steveharter
steveharter
commented
Sep 27, 2023

Copy link


Copy Markdown

Contributor
This touches the same files as the closed PR https://github.com/dotnet/runtime/pull/85156/files for issue #30024. It was closed since it didn't make sense to force this potentially risky issue late into v8.0. Both issues appear to be the same root cause around using the hashtable in s_defaultProviders, but have different use cases so it makes sense to keep open both issues for now.
Performance here will suffer quite a bit - because of the lock area expanding as well as the use of GetCustomAttributes(). The other PR should be used as a reference implementation to address perf, but there may be other ways to address the issues without the extensive changes there and added use the Environment.CurrentManagedThreadId.
@steveharter
steveharter
self-requested a review
September 27, 2023 21:21
@karakasa
karakasa
marked this pull request as draft
September 28, 2023 03:24
@karakasa
karakasa
marked this pull request as ready for review
September 29, 2023 05:25
@karakasa
karakasa
marked this pull request as draft
October 20, 2023 01:37
steveharter
steveharter
reviewed
Oct 20, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/ConcurrentTypeDescriptorTests.cs
Outdated

 public int OneProperty { get; set; }
 }
 }
}
@stevehartersteveharterOct 20, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Do you think the tests at https://github.com/dotnet/runtime/pull/85156/files#diff-13f56d055c80679284090c59b45d396ab44378385c1f7ad0b597eec15fa5cf0fR813-R909 add any value here?
FWIW locally I added those tests to this file, and ran them in loop for 200 times are no issues were found.
@karakasakarakasaNov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The other tests are not fundamentally different - but I add them if they produce errors on your computer and they don't use Thread (compared to my tests), which is unsupported on browsers.
Nevertheless, both mine and the other tests failed on my PC. Can the issue be platform-specific?
test log
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyClass), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter))​5ms
Error:
Assert.All() Failure: 1 out of 200 items in the collection did not pass.
[2]: Item: System.ComponentModel.TypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Actual: System.ComponentModel.TypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProvider), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter))​2ms
Error:
Assert.All() Failure: 11 out of 200 items in the collection did not pass.
[11]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[10]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[9]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[8]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[7]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[6]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[5]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[4]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[3]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[2]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[0]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected​25ms
Error:
Fallback type descriptor is used. Possible race condition.
Expected: False
Actual: True
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected() in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1287
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in D:\repos\karakasa\runtime\src\coreclr\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.CoreCLR.cs:line 36
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs:line 57

steveharter
steveharter
suggested changes
Oct 20, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left some comments in the test file.
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 20, 2023
@steveharter
steveharter
commented
Oct 23, 2023

Copy link


Copy Markdown

Contributor
Note the PR is still marked as Draft.
@karakasa
karakasa
commented
Oct 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Note the PR is still marked as Draft.
I am testing on adding another HashTable to reduce lock. Will mark the PR as ready once I decide whether to add the extra hashtable. Do you have thoughts on this?
I will also check the extra tests, thank you.
public sealed class TypeDescriptor
{
....
private static readonly Hashtable s_defaultProviders = new Hashtable();
private static readonly Hashtable s_defaultProvidersCreated = new Hashtable();
....
private static void CheckDefaultProvider(Type type)
{
bool providerAdded = false;
if (s_defaultProvidersCreated.ContainsKey(type))
{
return;
}
lock (s_internalSyncObject)
{
if (s_defaultProviders.ContainsKey(type))
{
return;
}
object[] attrs = type.GetCustomAttributes(typeof(TypeDescriptionProviderAttribute), false);
for (int idx = attrs.Length - 1; idx >= 0; idx--)
{
....
}
s_defaultProvidersCreated[type] = null;
}
....
}
....
}

@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 25, 2023
@stevehartersteveharter
mentioned this pull request
Oct 26, 2023

Closed

karakasaand others
added 2 commits
November 15, 2023 22:47
@karakasa@Maximys
Added another HashTable to reduce locks.
8a7df35
Moved tests to the main testing file
Adopted tests from dotnet#85156
Co-authored-by: Maximys <mixim33@yandex.ru>
@karakasa
better test names
0e3fc99
@karakasa
karakasa
commented
Nov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
latest benchmark with an extra hashtable to reduce locks. The perf is on par with the main repo.
BenchmarkDotNet v0.13.8, Windows 10 (10.0.19045.2546/22H2/2022Update)
Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.100-rc.2.23502.2
[Host] : .NET 8.0.0 (8.0.23.47906), X64 RyuJIT AVX2
Job-IOOUIL : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-QTNBWJ : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

MethodToolchainMeanErrorStdDevRatioGen0Gen1Gen2AllocatedAlloc Ratio
MultipleThreadsmain1,987.632 μs14.8732 μs13.9124 μs1.0035.15637.81257.812510.39 KB1.00
MultipleThreadspr1,959.046 μs21.4355 μs20.0508 μs0.9935.15637.81257.812510.39 KB1.00
SingleThreadmain5.042 μs0.0173 μs0.0145 μs1.000.3052--1.92 KB1.00
SingleThreadpr5.061 μs0.0664 μs0.0621 μs1.000.3052--1.92 KB1.00
@karakasa
karakasa
marked this pull request as ready for review
November 15, 2023 16:10
@karakasa
karakasa
marked this pull request as draft
November 16, 2023 02:21
@karakasa
Skip one test on browsers
33d5a3e
ConcurrentGetProperties_ReturnsExpected
is skipped on browsers because Thread.Start
is unsupported.
@karakasa
karakasa
marked this pull request as ready for review
November 16, 2023 03:59
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 16, 2023

Closed

@ericstjericstj
mentioned this pull request
Dec 12, 2023

Closed

@ericstj
ericstj
commented
Dec 12, 2023

Copy link


Copy Markdown

Member
Before seeing this PR I happened to be looking at this problem and made a suggestion in #30024. I think that suggestion could be considered here as a slight alternative to what's been implemented so far.
@stevehartersteveharter
mentioned this pull request
Jan 11, 2024
 Merged

@steveharter
steveharter
commented
Jan 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Replaced by #96846 so we can apply the sentinel pattern mentioned in #30024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 11, 2024
@karakasa
karakasa
deleted the

issue-92394

branch
May 25, 2024 08:45
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stevehartersteveharterAwaiting requested review from steveharter

Assignees

@stevehartersteveharter

Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

TypeDescriptor.GetProperties(object instance) is not thread-safe


3 participants

@karakasa@steveharter@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Make TypeDescriptor thread safe with custom providers by enlarging lock region - #92521
Closed
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Closed
Make TypeDescriptor thread safe with custom providers by enlarging lock region#92521
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Conversation
@karakasa
@karakasakarakasa
commented
Sep 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Summary
Fix#92394 and add a test.
see comment for the reason of race conditions.
The fix is conservative as it just extends the range of lock statement. If the performance decrease in this PR is acceptable, I would recommend enlarging lock region because it is unlikely to result in bugs.
I did code another aggressive approach (#92548) that minimizes lock use but it seems causing stack overflow in certain conditions so I'm not gonna PR that.
Risk
There would be performance impact due to the extended use of lock.
The bottleneck would be querying many to-be-created TypeDescriptionNodes at the same time - as they are forced to be created one by one - which may decrease startup perf. Otherwise the impact should be minimal because most queries quit at the first if block.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Sep 23, 2023
@ghost
ghost
commented
Sep 23, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
Fix #92394 and add a test.
Author:karakasa
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@karakasakarakasa
changed the title
Make TypeDescriptor.GetProperties thread safeMake TypeDescriptor thread safeSep 23, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safeMake TypeDescriptor thread safe with custom providersSep 23, 2023
@karakasa
Make TypeDescriptor thread safe with custom providers
a8a259c
Originally a race condition exists in `CheckDefaultProvider` and leads
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92934
@karakasa
Originally a race condition exists in CheckDefaultProvider and leads
e4c4296
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92394
@karakasa
karakasa
marked this pull request as ready for review
September 23, 2023 10:50
@karakasakarakasa
mentioned this pull request
Sep 24, 2023
 Closed

@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providersMake TypeDescriptor thread safe with custom providers by enlaring lock regionSep 24, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providers by enlaring lock regionMake TypeDescriptor thread safe with custom providers by enlarging lock regionSep 24, 2023
@steveharter
steveharter
commented
Sep 27, 2023

Copy link


Copy Markdown

Contributor
This touches the same files as the closed PR https://github.com/dotnet/runtime/pull/85156/files for issue #30024. It was closed since it didn't make sense to force this potentially risky issue late into v8.0. Both issues appear to be the same root cause around using the hashtable in s_defaultProviders, but have different use cases so it makes sense to keep open both issues for now.
Performance here will suffer quite a bit - because of the lock area expanding as well as the use of GetCustomAttributes(). The other PR should be used as a reference implementation to address perf, but there may be other ways to address the issues without the extensive changes there and added use the Environment.CurrentManagedThreadId.
@steveharter
steveharter
self-requested a review
September 27, 2023 21:21
@karakasa
karakasa
marked this pull request as draft
September 28, 2023 03:24
@karakasa
karakasa
marked this pull request as ready for review
September 29, 2023 05:25
@karakasa
karakasa
marked this pull request as draft
October 20, 2023 01:37
steveharter
steveharter
reviewed
Oct 20, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/ConcurrentTypeDescriptorTests.cs
Outdated

 public int OneProperty { get; set; }
 }
 }
}
@stevehartersteveharterOct 20, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Do you think the tests at https://github.com/dotnet/runtime/pull/85156/files#diff-13f56d055c80679284090c59b45d396ab44378385c1f7ad0b597eec15fa5cf0fR813-R909 add any value here?
FWIW locally I added those tests to this file, and ran them in loop for 200 times are no issues were found.
@karakasakarakasaNov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The other tests are not fundamentally different - but I add them if they produce errors on your computer and they don't use Thread (compared to my tests), which is unsupported on browsers.
Nevertheless, both mine and the other tests failed on my PC. Can the issue be platform-specific?
test log
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyClass), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter))​5ms
Error:
Assert.All() Failure: 1 out of 200 items in the collection did not pass.
[2]: Item: System.ComponentModel.TypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Actual: System.ComponentModel.TypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProvider), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter))​2ms
Error:
Assert.All() Failure: 11 out of 200 items in the collection did not pass.
[11]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[10]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[9]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[8]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[7]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[6]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[5]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[4]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[3]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[2]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[0]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected​25ms
Error:
Fallback type descriptor is used. Possible race condition.
Expected: False
Actual: True
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected() in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1287
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in D:\repos\karakasa\runtime\src\coreclr\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.CoreCLR.cs:line 36
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs:line 57

steveharter
steveharter
suggested changes
Oct 20, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left some comments in the test file.
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 20, 2023
@steveharter
steveharter
commented
Oct 23, 2023

Copy link


Copy Markdown

Contributor
Note the PR is still marked as Draft.
@karakasa
karakasa
commented
Oct 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Note the PR is still marked as Draft.
I am testing on adding another HashTable to reduce lock. Will mark the PR as ready once I decide whether to add the extra hashtable. Do you have thoughts on this?
I will also check the extra tests, thank you.
public sealed class TypeDescriptor
{
....
private static readonly Hashtable s_defaultProviders = new Hashtable();
private static readonly Hashtable s_defaultProvidersCreated = new Hashtable();
....
private static void CheckDefaultProvider(Type type)
{
bool providerAdded = false;
if (s_defaultProvidersCreated.ContainsKey(type))
{
return;
}
lock (s_internalSyncObject)
{
if (s_defaultProviders.ContainsKey(type))
{
return;
}
object[] attrs = type.GetCustomAttributes(typeof(TypeDescriptionProviderAttribute), false);
for (int idx = attrs.Length - 1; idx >= 0; idx--)
{
....
}
s_defaultProvidersCreated[type] = null;
}
....
}
....
}

@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 25, 2023
@stevehartersteveharter
mentioned this pull request
Oct 26, 2023

Closed

karakasaand others
added 2 commits
November 15, 2023 22:47
@karakasa@Maximys
Added another HashTable to reduce locks.
8a7df35
Moved tests to the main testing file
Adopted tests from dotnet#85156
Co-authored-by: Maximys <mixim33@yandex.ru>
@karakasa
better test names
0e3fc99
@karakasa
karakasa
commented
Nov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
latest benchmark with an extra hashtable to reduce locks. The perf is on par with the main repo.
BenchmarkDotNet v0.13.8, Windows 10 (10.0.19045.2546/22H2/2022Update)
Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.100-rc.2.23502.2
[Host] : .NET 8.0.0 (8.0.23.47906), X64 RyuJIT AVX2
Job-IOOUIL : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-QTNBWJ : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

MethodToolchainMeanErrorStdDevRatioGen0Gen1Gen2AllocatedAlloc Ratio
MultipleThreadsmain1,987.632 μs14.8732 μs13.9124 μs1.0035.15637.81257.812510.39 KB1.00
MultipleThreadspr1,959.046 μs21.4355 μs20.0508 μs0.9935.15637.81257.812510.39 KB1.00
SingleThreadmain5.042 μs0.0173 μs0.0145 μs1.000.3052--1.92 KB1.00
SingleThreadpr5.061 μs0.0664 μs0.0621 μs1.000.3052--1.92 KB1.00
@karakasa
karakasa
marked this pull request as ready for review
November 15, 2023 16:10
@karakasa
karakasa
marked this pull request as draft
November 16, 2023 02:21
@karakasa
Skip one test on browsers
33d5a3e
ConcurrentGetProperties_ReturnsExpected
is skipped on browsers because Thread.Start
is unsupported.
@karakasa
karakasa
marked this pull request as ready for review
November 16, 2023 03:59
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 16, 2023

Closed

@ericstjericstj
mentioned this pull request
Dec 12, 2023

Closed

@ericstj
ericstj
commented
Dec 12, 2023

Copy link


Copy Markdown

Member
Before seeing this PR I happened to be looking at this problem and made a suggestion in #30024. I think that suggestion could be considered here as a slight alternative to what's been implemented so far.
@stevehartersteveharter
mentioned this pull request
Jan 11, 2024
 Merged

@steveharter
steveharter
commented
Jan 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Replaced by #96846 so we can apply the sentinel pattern mentioned in #30024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 11, 2024
@karakasa
karakasa
deleted the

issue-92394

branch
May 25, 2024 08:45
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stevehartersteveharterAwaiting requested review from steveharter

Assignees

@stevehartersteveharter

Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

TypeDescriptor.GetProperties(object instance) is not thread-safe


3 participants

@karakasa@steveharter@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make TypeDescriptor thread safe with custom providers by enlarging lock region - #92521
Closed
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Closed
Make TypeDescriptor thread safe with custom providers by enlarging lock region#92521
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Conversation
@karakasa
@karakasakarakasa
commented
Sep 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Summary
Fix#92394 and add a test.
see comment for the reason of race conditions.
The fix is conservative as it just extends the range of lock statement. If the performance decrease in this PR is acceptable, I would recommend enlarging lock region because it is unlikely to result in bugs.
I did code another aggressive approach (#92548) that minimizes lock use but it seems causing stack overflow in certain conditions so I'm not gonna PR that.
Risk
There would be performance impact due to the extended use of lock.
The bottleneck would be querying many to-be-created TypeDescriptionNodes at the same time - as they are forced to be created one by one - which may decrease startup perf. Otherwise the impact should be minimal because most queries quit at the first if block.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Sep 23, 2023
@ghost
ghost
commented
Sep 23, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
Fix #92394 and add a test.
Author:karakasa
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@karakasakarakasa
changed the title
Make TypeDescriptor.GetProperties thread safeMake TypeDescriptor thread safeSep 23, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safeMake TypeDescriptor thread safe with custom providersSep 23, 2023
@karakasa
Make TypeDescriptor thread safe with custom providers
a8a259c
Originally a race condition exists in `CheckDefaultProvider` and leads
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92934
@karakasa
Originally a race condition exists in CheckDefaultProvider and leads
e4c4296
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92394
@karakasa
karakasa
marked this pull request as ready for review
September 23, 2023 10:50
@karakasakarakasa
mentioned this pull request
Sep 24, 2023
 Closed

@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providersMake TypeDescriptor thread safe with custom providers by enlaring lock regionSep 24, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providers by enlaring lock regionMake TypeDescriptor thread safe with custom providers by enlarging lock regionSep 24, 2023
@steveharter
steveharter
commented
Sep 27, 2023

Copy link


Copy Markdown

Contributor
This touches the same files as the closed PR https://github.com/dotnet/runtime/pull/85156/files for issue #30024. It was closed since it didn't make sense to force this potentially risky issue late into v8.0. Both issues appear to be the same root cause around using the hashtable in s_defaultProviders, but have different use cases so it makes sense to keep open both issues for now.
Performance here will suffer quite a bit - because of the lock area expanding as well as the use of GetCustomAttributes(). The other PR should be used as a reference implementation to address perf, but there may be other ways to address the issues without the extensive changes there and added use the Environment.CurrentManagedThreadId.
@steveharter
steveharter
self-requested a review
September 27, 2023 21:21
@karakasa
karakasa
marked this pull request as draft
September 28, 2023 03:24
@karakasa
karakasa
marked this pull request as ready for review
September 29, 2023 05:25
@karakasa
karakasa
marked this pull request as draft
October 20, 2023 01:37
steveharter
steveharter
reviewed
Oct 20, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/ConcurrentTypeDescriptorTests.cs
Outdated

 public int OneProperty { get; set; }
 }
 }
}
@stevehartersteveharterOct 20, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Do you think the tests at https://github.com/dotnet/runtime/pull/85156/files#diff-13f56d055c80679284090c59b45d396ab44378385c1f7ad0b597eec15fa5cf0fR813-R909 add any value here?
FWIW locally I added those tests to this file, and ran them in loop for 200 times are no issues were found.
@karakasakarakasaNov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The other tests are not fundamentally different - but I add them if they produce errors on your computer and they don't use Thread (compared to my tests), which is unsupported on browsers.
Nevertheless, both mine and the other tests failed on my PC. Can the issue be platform-specific?
test log
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyClass), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter))​5ms
Error:
Assert.All() Failure: 1 out of 200 items in the collection did not pass.
[2]: Item: System.ComponentModel.TypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Actual: System.ComponentModel.TypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProvider), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter))​2ms
Error:
Assert.All() Failure: 11 out of 200 items in the collection did not pass.
[11]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[10]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[9]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[8]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[7]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[6]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[5]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[4]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[3]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[2]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[0]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected​25ms
Error:
Fallback type descriptor is used. Possible race condition.
Expected: False
Actual: True
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected() in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1287
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in D:\repos\karakasa\runtime\src\coreclr\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.CoreCLR.cs:line 36
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs:line 57

steveharter
steveharter
suggested changes
Oct 20, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left some comments in the test file.
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 20, 2023
@steveharter
steveharter
commented
Oct 23, 2023

Copy link


Copy Markdown

Contributor
Note the PR is still marked as Draft.
@karakasa
karakasa
commented
Oct 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Note the PR is still marked as Draft.
I am testing on adding another HashTable to reduce lock. Will mark the PR as ready once I decide whether to add the extra hashtable. Do you have thoughts on this?
I will also check the extra tests, thank you.
public sealed class TypeDescriptor
{
....
private static readonly Hashtable s_defaultProviders = new Hashtable();
private static readonly Hashtable s_defaultProvidersCreated = new Hashtable();
....
private static void CheckDefaultProvider(Type type)
{
bool providerAdded = false;
if (s_defaultProvidersCreated.ContainsKey(type))
{
return;
}
lock (s_internalSyncObject)
{
if (s_defaultProviders.ContainsKey(type))
{
return;
}
object[] attrs = type.GetCustomAttributes(typeof(TypeDescriptionProviderAttribute), false);
for (int idx = attrs.Length - 1; idx >= 0; idx--)
{
....
}
s_defaultProvidersCreated[type] = null;
}
....
}
....
}

@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 25, 2023
@stevehartersteveharter
mentioned this pull request
Oct 26, 2023

Closed

karakasaand others
added 2 commits
November 15, 2023 22:47
@karakasa@Maximys
Added another HashTable to reduce locks.
8a7df35
Moved tests to the main testing file
Adopted tests from dotnet#85156
Co-authored-by: Maximys <mixim33@yandex.ru>
@karakasa
better test names
0e3fc99
@karakasa
karakasa
commented
Nov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
latest benchmark with an extra hashtable to reduce locks. The perf is on par with the main repo.
BenchmarkDotNet v0.13.8, Windows 10 (10.0.19045.2546/22H2/2022Update)
Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.100-rc.2.23502.2
[Host] : .NET 8.0.0 (8.0.23.47906), X64 RyuJIT AVX2
Job-IOOUIL : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-QTNBWJ : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

MethodToolchainMeanErrorStdDevRatioGen0Gen1Gen2AllocatedAlloc Ratio
MultipleThreadsmain1,987.632 μs14.8732 μs13.9124 μs1.0035.15637.81257.812510.39 KB1.00
MultipleThreadspr1,959.046 μs21.4355 μs20.0508 μs0.9935.15637.81257.812510.39 KB1.00
SingleThreadmain5.042 μs0.0173 μs0.0145 μs1.000.3052--1.92 KB1.00
SingleThreadpr5.061 μs0.0664 μs0.0621 μs1.000.3052--1.92 KB1.00
@karakasa
karakasa
marked this pull request as ready for review
November 15, 2023 16:10
@karakasa
karakasa
marked this pull request as draft
November 16, 2023 02:21
@karakasa
Skip one test on browsers
33d5a3e
ConcurrentGetProperties_ReturnsExpected
is skipped on browsers because Thread.Start
is unsupported.
@karakasa
karakasa
marked this pull request as ready for review
November 16, 2023 03:59
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 16, 2023

Closed

@ericstjericstj
mentioned this pull request
Dec 12, 2023

Closed

@ericstj
ericstj
commented
Dec 12, 2023

Copy link


Copy Markdown

Member
Before seeing this PR I happened to be looking at this problem and made a suggestion in #30024. I think that suggestion could be considered here as a slight alternative to what's been implemented so far.
@stevehartersteveharter
mentioned this pull request
Jan 11, 2024
 Merged

@steveharter
steveharter
commented
Jan 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Replaced by #96846 so we can apply the sentinel pattern mentioned in #30024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 11, 2024
@karakasa
karakasa
deleted the

issue-92394

branch
May 25, 2024 08:45
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stevehartersteveharterAwaiting requested review from steveharter

Assignees

@stevehartersteveharter

Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

TypeDescriptor.GetProperties(object instance) is not thread-safe


3 participants

@karakasa@steveharter@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make TypeDescriptor thread safe with custom providers by enlarging lock region - #92521
Closed
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Closed
Make TypeDescriptor thread safe with custom providers by enlarging lock region#92521
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Conversation
@karakasa
@karakasakarakasa
commented
Sep 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Summary
Fix#92394 and add a test.
see comment for the reason of race conditions.
The fix is conservative as it just extends the range of lock statement. If the performance decrease in this PR is acceptable, I would recommend enlarging lock region because it is unlikely to result in bugs.
I did code another aggressive approach (#92548) that minimizes lock use but it seems causing stack overflow in certain conditions so I'm not gonna PR that.
Risk
There would be performance impact due to the extended use of lock.
The bottleneck would be querying many to-be-created TypeDescriptionNodes at the same time - as they are forced to be created one by one - which may decrease startup perf. Otherwise the impact should be minimal because most queries quit at the first if block.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Sep 23, 2023
@ghost
ghost
commented
Sep 23, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
Fix #92394 and add a test.
Author:karakasa
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@karakasakarakasa
changed the title
Make TypeDescriptor.GetProperties thread safeMake TypeDescriptor thread safeSep 23, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safeMake TypeDescriptor thread safe with custom providersSep 23, 2023
@karakasa
Make TypeDescriptor thread safe with custom providers
a8a259c
Originally a race condition exists in `CheckDefaultProvider` and leads
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92934
@karakasa
Originally a race condition exists in CheckDefaultProvider and leads
e4c4296
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92394
@karakasa
karakasa
marked this pull request as ready for review
September 23, 2023 10:50
@karakasakarakasa
mentioned this pull request
Sep 24, 2023
 Closed

@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providersMake TypeDescriptor thread safe with custom providers by enlaring lock regionSep 24, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providers by enlaring lock regionMake TypeDescriptor thread safe with custom providers by enlarging lock regionSep 24, 2023
@steveharter
steveharter
commented
Sep 27, 2023

Copy link


Copy Markdown

Contributor
This touches the same files as the closed PR https://github.com/dotnet/runtime/pull/85156/files for issue #30024. It was closed since it didn't make sense to force this potentially risky issue late into v8.0. Both issues appear to be the same root cause around using the hashtable in s_defaultProviders, but have different use cases so it makes sense to keep open both issues for now.
Performance here will suffer quite a bit - because of the lock area expanding as well as the use of GetCustomAttributes(). The other PR should be used as a reference implementation to address perf, but there may be other ways to address the issues without the extensive changes there and added use the Environment.CurrentManagedThreadId.
@steveharter
steveharter
self-requested a review
September 27, 2023 21:21
@karakasa
karakasa
marked this pull request as draft
September 28, 2023 03:24
@karakasa
karakasa
marked this pull request as ready for review
September 29, 2023 05:25
@karakasa
karakasa
marked this pull request as draft
October 20, 2023 01:37
steveharter
steveharter
reviewed
Oct 20, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/ConcurrentTypeDescriptorTests.cs
Outdated

 public int OneProperty { get; set; }
 }
 }
}
@stevehartersteveharterOct 20, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Do you think the tests at https://github.com/dotnet/runtime/pull/85156/files#diff-13f56d055c80679284090c59b45d396ab44378385c1f7ad0b597eec15fa5cf0fR813-R909 add any value here?
FWIW locally I added those tests to this file, and ran them in loop for 200 times are no issues were found.
@karakasakarakasaNov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The other tests are not fundamentally different - but I add them if they produce errors on your computer and they don't use Thread (compared to my tests), which is unsupported on browsers.
Nevertheless, both mine and the other tests failed on my PC. Can the issue be platform-specific?
test log
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyClass), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter))​5ms
Error:
Assert.All() Failure: 1 out of 200 items in the collection did not pass.
[2]: Item: System.ComponentModel.TypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Actual: System.ComponentModel.TypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProvider), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter))​2ms
Error:
Assert.All() Failure: 11 out of 200 items in the collection did not pass.
[11]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[10]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[9]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[8]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[7]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[6]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[5]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[4]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[3]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[2]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[0]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected​25ms
Error:
Fallback type descriptor is used. Possible race condition.
Expected: False
Actual: True
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected() in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1287
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in D:\repos\karakasa\runtime\src\coreclr\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.CoreCLR.cs:line 36
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs:line 57

steveharter
steveharter
suggested changes
Oct 20, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left some comments in the test file.
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 20, 2023
@steveharter
steveharter
commented
Oct 23, 2023

Copy link


Copy Markdown

Contributor
Note the PR is still marked as Draft.
@karakasa
karakasa
commented
Oct 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Note the PR is still marked as Draft.
I am testing on adding another HashTable to reduce lock. Will mark the PR as ready once I decide whether to add the extra hashtable. Do you have thoughts on this?
I will also check the extra tests, thank you.
public sealed class TypeDescriptor
{
....
private static readonly Hashtable s_defaultProviders = new Hashtable();
private static readonly Hashtable s_defaultProvidersCreated = new Hashtable();
....
private static void CheckDefaultProvider(Type type)
{
bool providerAdded = false;
if (s_defaultProvidersCreated.ContainsKey(type))
{
return;
}
lock (s_internalSyncObject)
{
if (s_defaultProviders.ContainsKey(type))
{
return;
}
object[] attrs = type.GetCustomAttributes(typeof(TypeDescriptionProviderAttribute), false);
for (int idx = attrs.Length - 1; idx >= 0; idx--)
{
....
}
s_defaultProvidersCreated[type] = null;
}
....
}
....
}

@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 25, 2023
@stevehartersteveharter
mentioned this pull request
Oct 26, 2023

Closed

karakasaand others
added 2 commits
November 15, 2023 22:47
@karakasa@Maximys
Added another HashTable to reduce locks.
8a7df35
Moved tests to the main testing file
Adopted tests from dotnet#85156
Co-authored-by: Maximys <mixim33@yandex.ru>
@karakasa
better test names
0e3fc99
@karakasa
karakasa
commented
Nov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
latest benchmark with an extra hashtable to reduce locks. The perf is on par with the main repo.
BenchmarkDotNet v0.13.8, Windows 10 (10.0.19045.2546/22H2/2022Update)
Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.100-rc.2.23502.2
[Host] : .NET 8.0.0 (8.0.23.47906), X64 RyuJIT AVX2
Job-IOOUIL : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-QTNBWJ : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

MethodToolchainMeanErrorStdDevRatioGen0Gen1Gen2AllocatedAlloc Ratio
MultipleThreadsmain1,987.632 μs14.8732 μs13.9124 μs1.0035.15637.81257.812510.39 KB1.00
MultipleThreadspr1,959.046 μs21.4355 μs20.0508 μs0.9935.15637.81257.812510.39 KB1.00
SingleThreadmain5.042 μs0.0173 μs0.0145 μs1.000.3052--1.92 KB1.00
SingleThreadpr5.061 μs0.0664 μs0.0621 μs1.000.3052--1.92 KB1.00
@karakasa
karakasa
marked this pull request as ready for review
November 15, 2023 16:10
@karakasa
karakasa
marked this pull request as draft
November 16, 2023 02:21
@karakasa
Skip one test on browsers
33d5a3e
ConcurrentGetProperties_ReturnsExpected
is skipped on browsers because Thread.Start
is unsupported.
@karakasa
karakasa
marked this pull request as ready for review
November 16, 2023 03:59
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 16, 2023

Closed

@ericstjericstj
mentioned this pull request
Dec 12, 2023

Closed

@ericstj
ericstj
commented
Dec 12, 2023

Copy link


Copy Markdown

Member
Before seeing this PR I happened to be looking at this problem and made a suggestion in #30024. I think that suggestion could be considered here as a slight alternative to what's been implemented so far.
@stevehartersteveharter
mentioned this pull request
Jan 11, 2024
 Merged

@steveharter
steveharter
commented
Jan 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Replaced by #96846 so we can apply the sentinel pattern mentioned in #30024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 11, 2024
@karakasa
karakasa
deleted the

issue-92394

branch
May 25, 2024 08:45
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stevehartersteveharterAwaiting requested review from steveharter

Assignees

@stevehartersteveharter

Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

TypeDescriptor.GetProperties(object instance) is not thread-safe


3 participants

@karakasa@steveharter@ericstj

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Make TypeDescriptor thread safe with custom providers by enlarging lock region - #92521
Closed
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Closed
Make TypeDescriptor thread safe with custom providers by enlarging lock region#92521
karakasa wants to merge 13 commits into
dotnet:mainfrom 
karakasa:issue-92394
Conversation
@karakasa
@karakasakarakasa
commented
Sep 23, 2023•

edited

Loading

Copy link


Copy Markdown

Contributor
Summary
Fix#92394 and add a test.
see comment for the reason of race conditions.
The fix is conservative as it just extends the range of lock statement. If the performance decrease in this PR is acceptable, I would recommend enlarging lock region because it is unlikely to result in bugs.
I did code another aggressive approach (#92548) that minimizes lock use but it seems causing stack overflow in certain conditions so I'm not gonna PR that.
Risk
There would be performance impact due to the extended use of lock.
The bottleneck would be querying many to-be-created TypeDescriptionNodes at the same time - as they are forced to be created one by one - which may decrease startup perf. Otherwise the impact should be minimal because most queries quit at the first if block.
@ghostghost
added

area-System.ComponentModel

community-contribution
Indicates that the PR has been added by a community member
labels
Sep 23, 2023
@ghost
ghost
commented
Sep 23, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-componentmodel

See info in area-owners.md if you want to be subscribed.
Issue Details
Fix #92394 and add a test.
Author:karakasa
Assignees:-
Labels:
area-System.ComponentModel
Milestone:-
@karakasakarakasa
changed the title
Make TypeDescriptor.GetProperties thread safeMake TypeDescriptor thread safeSep 23, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safeMake TypeDescriptor thread safe with custom providersSep 23, 2023
@karakasa
Make TypeDescriptor thread safe with custom providers
a8a259c
Originally a race condition exists in `CheckDefaultProvider` and leads
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92934
@karakasa
Originally a race condition exists in CheckDefaultProvider and leads
e4c4296
to wrong results when many methods are called simultaneously.
The PR fixes that by extending the lock statement.
Fixdotnet#92394
@karakasa
karakasa
marked this pull request as ready for review
September 23, 2023 10:50
@karakasakarakasa
mentioned this pull request
Sep 24, 2023
 Closed

@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providersMake TypeDescriptor thread safe with custom providers by enlaring lock regionSep 24, 2023
@karakasakarakasa
changed the title
Make TypeDescriptor thread safe with custom providers by enlaring lock regionMake TypeDescriptor thread safe with custom providers by enlarging lock regionSep 24, 2023
@steveharter
steveharter
commented
Sep 27, 2023

Copy link


Copy Markdown

Contributor
This touches the same files as the closed PR https://github.com/dotnet/runtime/pull/85156/files for issue #30024. It was closed since it didn't make sense to force this potentially risky issue late into v8.0. Both issues appear to be the same root cause around using the hashtable in s_defaultProviders, but have different use cases so it makes sense to keep open both issues for now.
Performance here will suffer quite a bit - because of the lock area expanding as well as the use of GetCustomAttributes(). The other PR should be used as a reference implementation to address perf, but there may be other ways to address the issues without the extensive changes there and added use the Environment.CurrentManagedThreadId.
@steveharter
steveharter
self-requested a review
September 27, 2023 21:21
@karakasa
karakasa
marked this pull request as draft
September 28, 2023 03:24
@karakasa
karakasa
marked this pull request as ready for review
September 29, 2023 05:25
@karakasa
karakasa
marked this pull request as draft
October 20, 2023 01:37
steveharter
steveharter
reviewed
Oct 20, 2023
View reviewed changes
Comment threadsrc/libraries/System.ComponentModel.TypeConverter/tests/ConcurrentTypeDescriptorTests.cs
Outdated

 public int OneProperty { get; set; }
 }
 }
}
@stevehartersteveharterOct 20, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Do you think the tests at https://github.com/dotnet/runtime/pull/85156/files#diff-13f56d055c80679284090c59b45d396ab44378385c1f7ad0b597eec15fa5cf0fR813-R909 add any value here?
FWIW locally I added those tests to this file, and ran them in loop for 200 times are no issues were found.
@karakasakarakasaNov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

The other tests are not fundamentally different - but I add them if they produce errors on your computer and they don't use Thread (compared to my tests), which is unsupported on browsers.
Nevertheless, both mine and the other tests failed on my PC. Can the issue be platform-specific?
test log
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyClass), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter))​5ms
Error:
Assert.All() Failure: 1 out of 200 items in the collection did not pass.
[2]: Item: System.ComponentModel.TypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Actual: System.ComponentModel.TypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(typeForGetConverter: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProvider), expectedConverterType: typeof(System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter))​2ms
Error:
Assert.All() Failure: 11 out of 200 items in the collection did not pass.
[11]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[10]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[9]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[8]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[7]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[6]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[5]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[4]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[3]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[2]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
[0]: Item: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
Xunit.Sdk.IsTypeException: Assert.IsType() Failure
Expected: System.ComponentModel.Tests.TypeDescriptorTests+MyInheritedClassWithCustomTypeDescriptionProviderConverter
Actual: System.ComponentModel.Tests.TypeDescriptorTests+MyTypeConverter
at Xunit.Assert.IsType(Type expectedType, Object object) in /_/src/Microsoft.DotNet.XUnitAssert/src/TypeAsserts.cs:line 140
at System.ComponentModel.Tests.TypeDescriptorTests.<>c__DisplayClass80_0.<GetConverterWithAddProvider_ByMultithread_Success>b__0(TypeConverter currentConverter) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1371
at Xunit.Assert.<>c__DisplayClass11_0`1.<All>b__0(T item, Int32 index) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 40
at Xunit.Assert.All[T](IEnumerable`1 collection, Action`2 action) in /_/src/Microsoft.DotNet.XUnitAssert/src/CollectionAsserts.cs:line 69
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetConverterWithAddProvider_ByMultithread_Success(Type typeForGetConverter, Type expectedConverterType) in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1370
at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Threading\Tasks\Task.cs:line 1914
✘ System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected​25ms
Error:
Fallback type descriptor is used. Possible race condition.
Expected: False
Actual: True
Stack trace:
at System.ComponentModel.Tests.TypeDescriptorTests.GetProperties_ReturnsExpected() in D:\repos\karakasa\runtime\src\libraries\System.ComponentModel.TypeConverter\tests\TypeDescriptorTests.cs:line 1287
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args) in D:\repos\karakasa\runtime\src\coreclr\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.CoreCLR.cs:line 36
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) in D:\repos\karakasa\runtime\src\libraries\System.Private.CoreLib\src\System\Reflection\MethodBaseInvoker.cs:line 57

steveharter
steveharter
suggested changes
Oct 20, 2023
View reviewed changes
@stevehartersteveharter
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Left some comments in the test file.
@ghostghost
added
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 20, 2023
@steveharter
steveharter
commented
Oct 23, 2023

Copy link


Copy Markdown

Contributor
Note the PR is still marked as Draft.
@karakasa
karakasa
commented
Oct 25, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Note the PR is still marked as Draft.
I am testing on adding another HashTable to reduce lock. Will mark the PR as ready once I decide whether to add the extra hashtable. Do you have thoughts on this?
I will also check the extra tests, thank you.
public sealed class TypeDescriptor
{
....
private static readonly Hashtable s_defaultProviders = new Hashtable();
private static readonly Hashtable s_defaultProvidersCreated = new Hashtable();
....
private static void CheckDefaultProvider(Type type)
{
bool providerAdded = false;
if (s_defaultProvidersCreated.ContainsKey(type))
{
return;
}
lock (s_internalSyncObject)
{
if (s_defaultProviders.ContainsKey(type))
{
return;
}
object[] attrs = type.GetCustomAttributes(typeof(TypeDescriptionProviderAttribute), false);
for (int idx = attrs.Length - 1; idx >= 0; idx--)
{
....
}
s_defaultProvidersCreated[type] = null;
}
....
}
....
}

@ghostghost
removed
the 
needs-author-action
An issue or pull request that requires more info or actions from the author.
label
Oct 25, 2023
@stevehartersteveharter
mentioned this pull request
Oct 26, 2023

Closed

karakasaand others
added 2 commits
November 15, 2023 22:47
@karakasa@Maximys
Added another HashTable to reduce locks.
8a7df35
Moved tests to the main testing file
Adopted tests from dotnet#85156
Co-authored-by: Maximys <mixim33@yandex.ru>
@karakasa
better test names
0e3fc99
@karakasa
karakasa
commented
Nov 15, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
latest benchmark with an extra hashtable to reduce locks. The perf is on par with the main repo.
BenchmarkDotNet v0.13.8, Windows 10 (10.0.19045.2546/22H2/2022Update)
Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.100-rc.2.23502.2
[Host] : .NET 8.0.0 (8.0.23.47906), X64 RyuJIT AVX2
Job-IOOUIL : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2
Job-QTNBWJ : .NET 9.0.0 (42.42.42.42424), X64 RyuJIT AVX2

MethodToolchainMeanErrorStdDevRatioGen0Gen1Gen2AllocatedAlloc Ratio
MultipleThreadsmain1,987.632 μs14.8732 μs13.9124 μs1.0035.15637.81257.812510.39 KB1.00
MultipleThreadspr1,959.046 μs21.4355 μs20.0508 μs0.9935.15637.81257.812510.39 KB1.00
SingleThreadmain5.042 μs0.0173 μs0.0145 μs1.000.3052--1.92 KB1.00
SingleThreadpr5.061 μs0.0664 μs0.0621 μs1.000.3052--1.92 KB1.00
@karakasa
karakasa
marked this pull request as ready for review
November 15, 2023 16:10
@karakasa
karakasa
marked this pull request as draft
November 16, 2023 02:21
@karakasa
Skip one test on browsers
33d5a3e
ConcurrentGetProperties_ReturnsExpected
is skipped on browsers because Thread.Start
is unsupported.
@karakasa
karakasa
marked this pull request as ready for review
November 16, 2023 03:59
@build-analysisbuild-analysisBot
mentioned this pull request
Nov 16, 2023

Closed

@ericstjericstj
mentioned this pull request
Dec 12, 2023

Closed

@ericstj
ericstj
commented
Dec 12, 2023

Copy link


Copy Markdown

Member
Before seeing this PR I happened to be looking at this problem and made a suggestion in #30024. I think that suggestion could be considered here as a slight alternative to what's been implemented so far.
@stevehartersteveharter
mentioned this pull request
Jan 11, 2024
 Merged

@steveharter
steveharter
commented
Jan 11, 2024•

edited

Loading

Copy link


Copy Markdown

Contributor
Replaced by #96846 so we can apply the sentinel pattern mentioned in #30024
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Feb 11, 2024
@karakasa
karakasa
deleted the

issue-92394

branch
May 25, 2024 08:45
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stevehartersteveharterAwaiting requested review from steveharter

Assignees

@stevehartersteveharter

Labels

area-System.ComponentModelcommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

TypeDescriptor.GetProperties(object instance) is not thread-safe


3 participants

@karakasa@steveharter@ericstj