-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[EventSource] Block EventCommandExecuted callback during initialization #105341
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
Conversation
|
Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti |
tommcdon
left a comment
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!
noahfalk
left a comment
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
noahfalk
left a comment
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
davmason
left a comment
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
RuntimeEventSourceis initialized, kicking off EventSource initialization where all deferred commandsm_deferredCommandsfor that EventSource instance are processed one at a time at the end ofEventSource.Initialize.However, should a derived EventSource set its callback
EventCommandExecutedwithin 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
RuntimeEventSourceduring initialization (e.g.m_deferredCommands = [ Update, Update, Update, ... ]), then becauseRuntimeEventSourceinitializes 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_eventCommandExecutedduringEventSource.Initializeshould the more-derived EventSource class set-up itsEventCommandExecutedcallback within itsOnEventCommand.To do so, we defer invoking the
EventCommandExecutedcallback on deferred commands until after all deferred commands are first-processed during initialization.Fixes #94964
Fixes #96324
Fixes #99497
Fixes #92519