-
Notifications
You must be signed in to change notification settings - Fork 5k
Fix concurrency issue in TypeDescriptor GetProperties() and GetConverter() #96846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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. Fix dotnet#92934
to wrong results when many methods are called simultaneously. The PR fixes that by extending the lock statement. Fix dotnet#92394
Moved tests to the main testing file Adopted tests from dotnet#85156 Co-authored-by: Maximys <mixim33@yandex.ru>
ConcurrentGetProperties_ReturnsExpected is skipped on browsers because Thread.Start is unsupported.
Tagging subscribers to this area: @dotnet/area-system-componentmodel Issue DetailsReplaces (and is derived from the same branch as) PR #92548 in order to apply the "sentinel" pattern so that we don't need a new Hashtable instance. Thanks karakasa and Maximys for your work on this.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, just one functional suggestion.
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. For those tests - assume you saw them failing before the fix and confirmed the fix fixes them?
Yes, I reverted the changes from the previous PR and the 3 new tests failed. Then I added back the changes and the tests passed. |
Fixes #92394
Fixes #30024
Replaces (and is derived from the same branch as) PR #92521 in order to apply the "sentinel" pattern so that we don't need a new Hashtable instance. Thanks karakasa and Maximys for your work on this.