-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix Deadlock Inside Metrics Code #105259
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
Fix Deadlock Inside Metrics Code #105259
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| using System.Collections.Generic; | ||
| using System.ComponentModel; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| namespace System.Diagnostics.Metrics | ||
| { | ||
|
|
@@ -88,6 +89,17 @@ protected void Publish() | |
| return; | ||
| } | ||
|
|
||
| // MeterListener has a static constructor that creates runtime metrics instruments. | ||
| // We need to ensure this static constructor is called before starting to publish the instrument. | ||
| // This is necessary because creating runtime metrics instruments will cause re-entry to the Publish method, | ||
| // potentially resulting in a deadlock due to the SyncObject lock. | ||
| // Sequence of the deadlock: | ||
| // 1. An application creates an early instrument (e.g., Counter) before the MeterListener static constructor is executed. | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we can consider moving the initialization later out of the static constructor.
tarekgh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| List<MeterListener>? allListeners = null; | ||
| lock (Instrument.SyncObject) | ||
| { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.