-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add MetadataUpdateHandlerAttribute #50954
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
12f356c
to
659961e
Compare
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.
Other than a one comment, it looks good.
Looks like you also did the reflection caching clearing too. Thank!
src/libraries/System.Runtime.Loader/tests/MetadataUpdateHandlerAttributeTest.cs
Show resolved
Hide resolved
...ries/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdateHandlerAttribute.cs
Show resolved
Hide resolved
...ries/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdateHandlerAttribute.cs
Show resolved
Hide resolved
...ries/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdateHandlerAttribute.cs
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System/Reflection/ReflectionCacheTests.cs
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System/Reflection/ReflectionCacheTests.cs
Show resolved
Hide resolved
659961e
to
f128dfc
Compare
And at least the beginning of reflection cache clearing support.
f128dfc
to
693c22b
Compare
@pranavkm, I was just looking at dotnet-watch to see what we'll need to do there to respect this, and it seems like we likely first need Microsoft.CodeAnalysis.ExternalAccess.Watch.Api.WatchHotReloadService.Update augmented to include type names, then we can modify the manager to forward those to the agent, and then the agent can use those to resolve the Types to be passed to the update handlers? Or is the plan to parse the metadata delta in the agent to extract updated type information? |
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.
LGTM - just one comment about updating the ref file.
Co-authored-by: Eric Erhardt <[email protected]>
|
||
// Create a new, empty cache to replace the old one and try to substitute it in. | ||
var newCache = new RuntimeTypeCache(this); | ||
if (ReferenceEquals(GCHandle.InternalCompareExchange(m_cache, newCache, existingCache), existingCache)) |
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.
It should be enough to just set the GCHandle target to null. The GCHandle is weakhandle and so the code should be generally prepared to handle the null case.
And at least the beginning of reflection cache clearing support.
Fixes #49361
cc: @tommcdon, @mikem8361, @steveharter, @pranavkm