Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call TypeDescriptor.GetProperties before calling Refresh in ReflectionCachesUpdateHandler #51604

Closed
wants to merge 2 commits into from

Conversation

stephentoub
Copy link
Member

Try to fix #51588

According to the docs:
"Before you make a call to the Refresh method to clear the cache, you need to call the GetProperties method for the specific assembly to cache the information first."

Counterintuitive, but ok.

cc: @jeffhandley, @safern

@ghost
Copy link

ghost commented Apr 21, 2021

Tagging subscribers to this area: @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

Try to fix #51588

According to the docs:
"Before you make a call to the Refresh method to clear the cache, you need to call the GetProperties method for the specific assembly to cache the information first."

Counterintuitive, but ok.

cc: @jeffhandley, @safern

Author: stephentoub
Assignees: -
Labels:

area-System.ComponentModel

Milestone: -

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

Hmm interesting 😕

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Thanks! Any idea why it was only failing on macOS?

@stephentoub
Copy link
Member Author

Thanks! Any idea why it was only failing on macOS?

My guess is the failures are happening when this test runs concurrently with some other test, and without the GetProperties call, Refresh ends up not being thread-safe. At least that's my very limited understanding from the docs and a quick perusal of the source. If that's true, then this isn't limited to just macOS, and that's simply the only place we've seen it so far.

@jeffhandley
Copy link
Member

Seems like a reasonable hunch. Thanks for addressing it so swiftly!

@danmoseley
Copy link
Member

Looks like it's also failed in Linux now so not Mac specific

@stephentoub
Copy link
Member Author

And also not fixed by this change. It's possible TypeDescriptor.Refresh simply isn't thread-safe, violating rules around statics in .NET. I'll need to look deeper.

TypeDescriptor.Refresh isn't thread-safe, trying to enumerate a hashtable that's concurrently mutated, leading to a concurrent modification exceptions.  We'll need to decide if we're ok with the limitation or if we need to do surgery on TypeDescriptor to fix it.
@stephentoub stephentoub deleted the fixtcreflectioncache branch April 21, 2021 13:46
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants