- Notifications
You must be signed in to change notification settings - Fork5.2k
[release/9.0-staging] Fix handling of IDynamicInterfaceCastable wrt CastCache#110007
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[release/9.0-staging] Fix handling of IDynamicInterfaceCastable wrt CastCache#110007
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Fixes#108229.The actual fix is the addition of an `if` check where it originally wasn't. I also fixed the other checks for consistency - positive checks are fine to cache, and negative checks against non-interface targets are also fine to cache.
Tagging subscribers to this area:@agocke,@MichalStrehovsky,@jkotas |
jeffschwMSFT left a comment
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. we are holding on this one but may take with the other IDynamicInterfaceCastable issues for consideration
1031b6e intorelease/9.0-stagingUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Backport of#108328 to release/9.0-staging
/cc@MichalStrehovsky
Customer Impact
One more IDynamicInterfaceCastable related issue. This one was found as we were unblocking tests as part of fixing a customer reported issue (that was known and had known disabled tests). The tests happened to also hit this problem.
The issue is that a cast of a IDynamicInterfaceCastable object to a variant interface would store the wrong value into CastCache - we'd answer the first cast correctly, but a second time the same pair of type is checked for castability, we'd use the wrong cached value and invalid cast exception would happen.
Regression
Regressed in .NET 8, likely in#90234.
Testing
Added targeted test that is part of this PR. We also subsequently enabled more testing in#108376 but that depends on both PRs so it's in a separate PR.
Risk
This should be low risk, it only affects IDynamicInterfaceCastable and we made sure the caching logic is same as CoreCLR. This has been in main for 2 months.
IMPORTANT: If this backport is for a servicing release, please verify that:
The PR target branch is
release/X.0-staging, notrelease/X.0.If the change touches code that ships in a NuGet package, you have added the necessarypackage authoring and gotten it explicitly reviewed.