Skip to content
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

[release/8.0-staging] Fixes deadlock for IncrementingPollingCounter callbacks #108648

Merged
merged 1 commit into from
Oct 11, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ namespace System.Diagnostics.Tracing
#endif
internal sealed class CounterGroup
{
// This is the opt-in switch for a .NET 8 backport of https://github.com/dotnet/runtime/pull/105548
private static bool CounterCallbackOnTimerThread { get; } =
AppContext.TryGetSwitch("System.Diagnostics.Tracing.CounterCallbackOnTimerThread", out bool value) ? value : false;

private readonly EventSource _eventSource;
private readonly List<DiagnosticCounter> _counters;
private static readonly object s_counterGroupLock = new object();
Expand Down Expand Up @@ -189,6 +193,16 @@ private void DisableTimer()
Debug.Assert(Monitor.IsEntered(s_counterGroupLock));
_pollingIntervalInMilliseconds = 0;
s_counterGroupEnabledList?.Remove(this);

if (CounterCallbackOnTimerThread && s_needsResetIncrementingPollingCounters.Count > 0)
{
// if there are any pending callbacks for the timer thread, remove them now that the timer is being disabled
foreach (DiagnosticCounter diagnosticCounter in _counters)
{
if (diagnosticCounter is IncrementingPollingCounter pollingCounter)
s_needsResetIncrementingPollingCounters.Remove(pollingCounter);
}
}
}

private void ResetCounters()
Expand All @@ -203,7 +217,17 @@ private void ResetCounters()
}
else if (counter is IncrementingPollingCounter ipCounter)
{
ipCounter.UpdateMetric();
if (CounterCallbackOnTimerThread)
{
// IncrementingPollingCounters will be reset on timer thread
// We need this to avoid deadlocks caused by running IncrementingPollingCounter._totalValueProvider under EventListener.EventListenersLock
// This is an opt-in fix for servicing because there is a small chance this could break apps (most likely tests).
s_needsResetIncrementingPollingCounters.Add(ipCounter);
}
else
{
ipCounter.UpdateMetric();
}
}
else if (counter is EventCounter eCounter)
{
Expand Down Expand Up @@ -265,6 +289,10 @@ private void OnTimer()

private static List<CounterGroup>? s_counterGroupEnabledList;

// Stores a list of IncrementingPollingCounters that need to be reset on the timer thread. This field
// is only accessed when CallbackOnTimerThread is true.
private static List<IncrementingPollingCounter> s_needsResetIncrementingPollingCounters = [];

private static void PollForValues()
{
AutoResetEvent? sleepEvent = null;
Expand All @@ -274,6 +302,7 @@ private static void PollForValues()
// calling into the callbacks can cause a re-entrancy into CounterGroup.Enable()
// and result in a deadlock. (See https://github.com/dotnet/runtime/issues/40190 for details)
var onTimers = new List<CounterGroup>();
List<IncrementingPollingCounter>? countersToReset = null;
while (true)
{
int sleepDurationInMilliseconds = int.MaxValue;
Expand All @@ -292,7 +321,24 @@ private static void PollForValues()
millisecondsTillNextPoll = Math.Max(1, millisecondsTillNextPoll);
sleepDurationInMilliseconds = Math.Min(sleepDurationInMilliseconds, millisecondsTillNextPoll);
}

if (CounterCallbackOnTimerThread && s_needsResetIncrementingPollingCounters.Count > 0)
{
countersToReset = s_needsResetIncrementingPollingCounters;
s_needsResetIncrementingPollingCounters = [];
}
}

if (countersToReset != null)
{
foreach (IncrementingPollingCounter counter in countersToReset)
{
counter.UpdateMetric();
}

countersToReset = null;
}

foreach (CounterGroup onTimer in onTimers)
{
onTimer.OnTimer();
Expand Down
Loading