Fix Deadlock Inside Metrics Code#105259
Merged
tarekgh merged 1 commit intodotnet:mainfrom Jul 22, 2024
Merged
Conversation
ghost
added
the
Member
Author
|
@stephentoub @jkotas could you please help review this fix? Thanks! |
Member
|
Do you think this will fix #105189? |
Member
Author
Very possible. I'll close this one when I merge to see if this will show up again. |
noahfalk
approved these changes
Jul 22, 2024
| // 2. Instrument.Publish is called and enters the SyncObject lock. | ||
| // 3. Within the lock block, MeterListener is called, triggering its static constructor. | ||
| // 4. The static constructor creates runtime metrics instruments, causing re-entry to Instrument.Publish and leading to a deadlock. | ||
| RuntimeHelpers.RunClassConstructor(typeof(MeterListener).TypeHandle); |
Member
There was a problem hiding this comment.
I've been thinking about it and this seems OK - I didn't come up with another place where the MeterListener static constructor would be a problem. In the future if we did discover any more it might be worthwhile to move the runtime metrics initialization out of the static constructor and instead do it as a lazy initialization within the MeterListener instance constructor.
Member
Author
There was a problem hiding this comment.
Yes, we can consider moving the initialization later out of the static constructor.
jkotas
reviewed
Jul 23, 2024
src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs
Show resolved
Hide resolved
jkotas
reviewed
Jul 23, 2024
src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Instrument.cs
Show resolved
Hide resolved
jkotas
added a commit
to jkotas/runtime
that referenced
this pull request
Jul 26, 2024
- Trigger the RuntimeMetrics initialization only when actually needed in the MeterListener constructor. - Delete the lock-ordering workaround and wrong comment introduced in dotnet#105259. Trigger the RuntimeMetrics initialization only when actually needed should make the lock-ordering workarond unnecessary.
jkotas
added a commit
to jkotas/runtime
that referenced
this pull request
Jul 26, 2024
- Trigger the RuntimeMetrics initialization only when actually needed in the MeterListener constructor. - Delete the lock-ordering workaround and wrong comment introduced in dotnet#105259. Trigger the RuntimeMetrics initialization only when actually needed should make the lock-ordering workarond unnecessary.
tarekgh
pushed a commit
that referenced
this pull request
Jul 26, 2024
* Simplify initialization of RuntimeMetrics - Trigger the RuntimeMetrics initialization only when actually needed in the MeterListener constructor. - Delete the lock-ordering workaround and wrong comment introduced in #105259. Trigger the RuntimeMetrics initialization only when actually needed should make the lock-ordering workarond unnecessary. * Delete unnecessary static fields * Update src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/RuntimeMetrics.cs
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.
5 participants
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.
This is a regression from the change in #104680.
Thanks to @rokonec who caught it and suggested the fix too.