-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[EventSource] Block EventCommandExecuted callback during initialization #105341
[EventSource] Block EventCommandExecuted callback during initialization #105341
Conversation
Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti |
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!
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 fix looks like it works around the issue which is OK, but I think we could fix the root cause fairly easily. Comments inline.
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs
Outdated
Show resolved
Hide resolved
Guard EventCommandExecuted deferred commands callbacks with EventSource initialization to prevent CounterGroup's callback registration from seeing unexpected commands
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.
Functionally the code change looks fine but I recommended some tweaks to the comments.
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs
Outdated
Show resolved
Hide resolved
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.
Thanks for tracking this down!
During managed startup, the managed EventSource
RuntimeEventSource
is initialized, kicking off EventSource initialization where all deferred commandsm_deferredCommands
for that EventSource instance are processed one at a time at the end ofEventSource.Initialize
.However, should a derived EventSource set its callback
EventCommandExecuted
within itsOnEventCommand
, then because of the decision to invoke the callback on all deferred commands, the callback will be invoked before all deferred commands could be initially processed inEventSource.Initialize
.CounterGroup registers its callback during construction, so should multiple events be sent to
RuntimeEventSource
during initialization (e.g.m_deferredCommands = [ Update, Update, Update, ... ]
), then becauseRuntimeEventSource
initializes multiple event counters once the deferred command is processed at the end ofEventSource.Initialize()
((i.e.m_deferredCommands = [ Enable, Update, Update, ... ]
), then the callback is invoked on all deferred commands before they are converted from Update.This leads to an assertion error as demonstrated by
in #96324 (comment).
This PR aims to avoid double-invocations of the same callback
m_eventCommandExecuted
duringEventSource.Initialize
should the more-derived EventSource class set-up itsEventCommandExecuted
callback within itsOnEventCommand
.To do so, we defer invoking the
EventCommandExecuted
callback on deferred commands until after all deferred commands are first-processed during initialization.Fixes #94964
Fixes #96324
Fixes #99497
Fixes #92519