This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Prevent ImplicitTransform of index getter method (#41418)#41647
Merged
josalem merged 2 commits intoOct 11, 2019
Merged
Conversation
* Prevent TransformSpec from attempting to implicitly create a delegate for index properties * Add tests
|
I'm fine with merging this into 3.1 since it's evidently causing problems for ASP.NET |
Author
|
@danmosemsft have you heard anyone else running in to this issue besides the linked coreclr/aspnet issue? |
|
No. My assumption was that you are bringing this because it's being requested by ASP.NET. @rynowak ? @josalem for clarity could you reformat the top post per the template: dotnet/coreclr#26910 |
Author
|
You are correct, I was just seeing if you had heard of this independently from another source than the issue I linked. I'll update the PR text. |
Member
|
I wasn't aware of this at all. Based on the impact it sounds like we need to fix it, right? |
|
Approved for 3.1. Please read the Teams' channel info regarding freeze period. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Cherry-pick of #41418 to release/3.1
Fix for the exception thrown in dotnet/coreclr#26890 that is a fallout of the issue being fixed in
DiagnosticSourceEventSourcein conjunction with dotnet/aspnetcore#11730.small repro (click to expand)
Attach
dotnet traceto this app and turn on theMicrosoft-Diagnostics-DiagnosticSourceprovider.Root Cause of Exception:
The getter for the implicit
Itemproperty for the index operator takes an argument, andDiagnosticSourceEventSourcemakes the assumption that property getters don't take arguments. It tries to bind the resulting delegate of type[retval] get_Item(int32 index)to the typeFunc<TObject, TProperty>so the binding fails and we get the exception you see in dotnet/coreclr#26890.This change prevents
DiagnosticSourceEventSourcefrom attempting to serialize the implicitItemproperty by blocking index getters.Customer Impact
ASP.NET Core has used
DiagnosticSourceEventSourcefor their eventing. Since they changed their event types to inherit fromIReadOnlyCollectionin 3.0, they will all hit the exception in dotnet/coreclr#26890 when turned on causing a bad diagnostic experience if you do not explicitly specify a transform.Regression
I don't believe this is a regression as the code existed in the 2.0 time frame as well, but it was rarely encountered. The difference now being that after dotnet/aspnetcore#11730, the behavior is being hit routinely by ASP.NET Core when diagnosing an MVC app.
Risk
Minimal. This code prevents an exception from being thrown and prevents bad behavior without changing expected behavior.
Tests
A test were added to validate that this change prevents the implicit index getter from being serialized.
CC - @tommcdon @noahfalk