Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix EventSource Test Failures in CoreFX #16999

Merged
merged 5 commits into from
Mar 17, 2018

Conversation

brianrob
Copy link
Member

Addresses System.Diagnostics.Tracing failures in dotnet/corefx#28112.

Fixes a regression caused by #16861.

This PR addresses two issues:

  1. When producing metadata for EventPipe events, if we encounter a failure due to unsupported parameter types, an exception is thrown which results in an uninitialized EventSource. This PR fixes this by catching the exception and not registering any metadata for the event. This preserves the trace at the cost of the metadata for the event. NOTE: A viewer can use its own metadata if need be to decode the event.
  2. When writing events using TraceLogging Write, the IntPtr that contains the native handle to the event was stored on the NameInfo object. NameInfos live for the life of the process, which means that if the EventSource is disposed and re-created, it can be re-used. This means that once the EventSource is created and disposed, any NameInfos now contain invalid event handles, which results in an AV when writing to the EventPipe. This is fixed by having a ConcurrentDictionary<EventId, EventHandle> on each EventSource which makes the lifetime of the EventHandle the same as the EventSource itself.

@brianrob brianrob requested a review from stephentoub March 16, 2018 20:44
@brianrob
Copy link
Member Author

Run EventSource tests:
@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

@brianrob
Copy link
Member Author

Run EventSource tests:
@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

@stephentoub
Copy link
Member

stephentoub commented Mar 17, 2018

if we encounter a failure due to unsupported parameter types

Does this mean that there's a bug in the EventSource for the System.Net.Security library that had a failed EventSource test as a result of this?
https://mc.dot.net/#/user/dotnet-maestro-bot/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/c306bc2010a67731a13babd322e48b34bf10b3bc/workItem/System.Net.Security.Tests/analysis/xunit/System.Net.Security.Tests.LoggingTest~2FEventSource_EventsRaisedAsExpected

@brianrob
Copy link
Member Author

@dotnet-bot test Ubuntu x64 Checked corefx_baseline

@brianrob
Copy link
Member Author

@stephentoub, the bug is not in NetEventSource, but in the EventPipe support for EventSource. I suspect the issue is that EventPipe doesn't currently know how to serialize arrays, and NetEventSource serializes a byte[]. Prior to the TraceLogging changes I made earlier this week, this would silently fail. What's currently in master does not silently fail. Based on the code in System.Net that I looked at, I expect that the test failure you linked to will be fixed by this PR.

@brianrob
Copy link
Member Author

Run EventSource tests:
@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

@brianrob
Copy link
Member Author

Run was aborted: @dotnet-bot test Ubuntu x64 Checked corefx_baseline

@brianrob
Copy link
Member Author

The failures for Ubuntu x64 Checked Build and Test (Jit - CoreFx) are due to infrastructure issues. I have run this leg manually and confirmed that there are no related failures.

@brianrob brianrob merged commit 8a5464f into dotnet:master Mar 17, 2018
@brianrob brianrob deleted the eventsource_test_failures branch March 17, 2018 22:18
dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Mar 17, 2018
stephentoub pushed a commit to dotnet/corefx that referenced this pull request Mar 18, 2018
dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Mar 18, 2018
jkotas pushed a commit to dotnet/corert that referenced this pull request Mar 18, 2018
brianrob added a commit to brianrob/coreclr that referenced this pull request Mar 27, 2018
ericstj pushed a commit to ericstj/corefx that referenced this pull request Mar 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants