[EventSource] Block EventCommandExecuted callback during initialization#105341
Merged
mdh1418 merged 3 commits intodotnet:mainfrom Jul 26, 2024
Merged
Conversation
Contributor
|
Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti |
This was referenced Jul 23, 2024
noahfalk
reviewed
Jul 24, 2024
Member
noahfalk
left a comment
There was a problem hiding this comment.
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
This was referenced Jul 25, 2024
Open
Closed
Closed
noahfalk
approved these changes
Jul 25, 2024
Member
noahfalk
left a comment
There was a problem hiding this comment.
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
approved these changes
Jul 26, 2024
Contributor
davmason
left a comment
There was a problem hiding this comment.
Thanks for tracking this down!
This was referenced Jul 26, 2024
Build failure: Static graph-based restore failed with exit code .* but did not log an error.
#103526
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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