-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fix System.Reflection.TargetException: Non-static method requires a target #20572
Conversation
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 code looks good, but we should figure out if #33013 is related before porting (assuming shiproom prefers one combined fix vs. two separate fixes)
Above #33013 = https://github.com/dotnet/corefx/issues/33013 |
…arget Attempting to access the HasValue property of a nullable type through reflection tosses when the value is null. This was found to be happening in System.Diagnostics.Tracing by the Application Insights team via telemetry data and they raised the issue in corefx (issue 32606). This fixes the issue by avoiding reflection and directly checking if the nullable object is null. This is a port of #20350
@noahfalk Please take a look at this response: https://github.com/dotnet/corefx/issues/33013#issuecomment-445087043 |
Just getting back from vacation. From Vance's response on the other thread it sounds like that second issue is already addressed in TraceEvent and we are good to take this fix as-is. Thanks for looking into it. cc @vancem |
@dotnet-bot test this please |
@jorive Do you want this considered for servicing? If so, please fill out the template. |
Please add |
Please wait for branch to open approx 2-3 weeks. Ping me if I don't merge then. |
@danmosemsft It's been 19 days since this PR was approved, can this be merged now? |
@mmitche are branches open yet |
@danmosemsft Not yet, later today. |
@wtgodbe would you mind making sure all our approved PR'S in the core repos get merged, if I'm not around? Going on vac soon. |
@danmosemsft sure thing |
Description
Attempting to access the HasValue property of a nullable type through reflection tosses when the value is null. This was found to be happening in System.Diagnostics.Tracing by the Application Insights team via telemetry data and they raised the issue in corefx (issue 32606). This fixes the issue by avoiding reflection and directly checking if the nullable object is null.
This is a port of #20350
Customer Impact
The customer impact (to fill out the template) as follows:
Regression?
No
Risk
The root cause of the Application Insights bug is but in the EventSource logging code in .NET Core. This code has the ability to serialize various datatypes including the special System.Nullable type. The EventSource code that handles this case was broken for the case that the value is null. To our knowledge this is the first time a Nullable with a null value has been passed as a data value to EventSource.
The fix is small (~7 lines of code changed), and does not affect any other cases but the serialization of a Nullable by an EventSource. Thus the risk is very low (because it was broken before, it is unlikely that other scenarios were event executing the code that was changed. In addition this code is only active when EventSource logging is turned on, and EventSource is already set up to swallow exceptions that happen during logging (in general it is better to lose logging than crash the app). This further mitigates the risk (at worst, a mistake in this fix will cause loss of logging, not application crash).
Issue
https://github.com/dotnet/corefx/issues/32606