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

Don't capture AsyncLocals into EventCounter timer#26075

Merged
stephentoub merged 3 commits intodotnet:masterfrom
benaadams:eventcounter-asynclocals
Jan 30, 2018
Merged

Don't capture AsyncLocals into EventCounter timer#26075
stephentoub merged 3 commits intodotnet:masterfrom
benaadams:eventcounter-asynclocals

Conversation

@benaadams
Copy link
Member

@karelz
Copy link
Member

karelz commented Jan 18, 2018

@brianrob @vancem can you please take a look?

@karelz
Copy link
Member

karelz commented Jan 27, 2018

@brianrob @vancem ping?

restoreFlow = true;
}

_pollingTimer = new Timer(OnTimer, null, _pollingIntervalInMilliseconds, _pollingIntervalInMilliseconds);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new Timer(s => ((EventCounter)s).OnTimer(null), this, _pollingIntervalInMilliseconds, _pollingIntervalInMilliseconds);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

@stephentoub
Copy link
Member

The OnTimer that's being invoked makes calls to _eventSource.Write. That Write call will in turn invoke user code from any registered EventListener. Previously that would have been done in the context of the captured ExecutionContext; now it won't.

@vancem
Copy link

vancem commented Jan 29, 2018

To address Stephen's concern. It is true that this changes the executioncontext that EventListeners will see when EventCounter events are fired. However, and EventCounter event represents an aggregation of many things and thus its context is not really valuable (at it only represents the current thread). In addition the timer is started by the thread that turns on the eventCounter. For ETW this is an INJECTED thread (not likely to be useful), and for EventListeners, it is the thread that called the 'Enable' API (that is a thread that logically belongs to the monitoring infrastructure). This makes it even less likely to be useful.

Thus I am OK with the semantics of this change.

@benaadams
Copy link
Member Author

benaadams commented Jan 29, 2018

RedHat.73.Amd64.Open-Release-x64 / System.Reflection.Metadata.Tests.MetadataReaderTests

System.BadImageFormatException : Invalid COR20 header signature.

@benaadams
Copy link
Member Author

test Linux x64 Release Build

@vancem
Copy link

vancem commented Jan 29, 2018

@dotnet-bot test NETFX x86 Release Build

@stephentoub stephentoub merged commit bcc2940 into dotnet:master Jan 30, 2018
@benaadams benaadams deleted the eventcounter-asynclocals branch January 30, 2018 15:17
@karelz karelz added this to the 2.1.0 milestone Feb 4, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Don't capture AsyncLocals into EventCounter timer

* Harden Flow suppression

* Feedback


Commit migrated from dotnet/corefx@bcc2940
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants