Fix Deadlock Inside Metrics Code #105259
Merged
Fix Deadlock Inside Metrics Code #105259
Conversation
This file contains 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
|
@stephentoub @jkotas could you please help review this fix? Thanks! |
|
Do you think this will fix #105189? |
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); |
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.
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
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.
This is a regression from the change in #104680.
Thanks to @rokonec who caught it and suggested the fix too.