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

Fix infinite loop in Diagnostics CounterGroup OnTimer method. #53887

Conversation

lateralusX
Copy link
Member

@lateralusX lateralusX commented Jun 8, 2021

If a counter event source is disabled while in System.Diagnostics.Tracing.CounterGroup.OnTimer, thread could end up in infinite loop. This happens since _pollingIntervalInMilliseconds is set to 0 when disabled, meaning that we could fail to advance _nextPollingTimeStamp past now:

lock (s_counterGroupLock)
{
    _timeStampSinceCollectionStarted = now;
    do
    {
        _nextPollingTimeStamp += new TimeSpan(0, 0, 0, 0, _pollingIntervalInMilliseconds);
    } while (_nextPollingTimeStamp <= now);
}

Fix use the local copy of pollingIntervalInMilliseconds and also adds an additional safe quard to make sure it always gets advance event if disabled check on _eventSource.IsEnabled() and lock (s_counterGroupLock), since that case will set _pollingIntervalInMilliseconds to 0 as well even before loaded into local variable.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@lateralusX
Copy link
Member Author

//CC @josalem

@ghost
Copy link

ghost commented Jun 8, 2021

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

If a counter event source is disabled while in System.Diagnostics.Tracing.CounterGroup.OnTimer, thread could end up in infinite loop. This happens since _pollingIntervalInMilliseconds is set to 0 when disabled, meaning that we could fail to advance _nextPollingTimeStamp past now:

lock (s_counterGroupLock)
{
    _timeStampSinceCollectionStarted = now;
    do
    {
        _nextPollingTimeStamp += new TimeSpan(0, 0, 0, 0, _pollingIntervalInMilliseconds);
    } while (_nextPollingTimeStamp <= now);
}

Fix use the local copy of pollingIntervalInMilliseconds and also adds an additional safe quard to make sure it always gets advance event if disabled check on _eventSource.IsEnabled() and lock (s_counterGroupLock), since that case will set _pollingIntervalInMilliseconds to 0 as well even before loaded into local variable.

Author: lateralusX
Assignees: -
Labels:

area-System.Diagnostics.Tracing

Milestone: -

@lateralusX lateralusX changed the title Fix infinite loop in Diagnostics counter groups OnTimer method. Fix infinite loop in Diagnostics CounterGroup OnTimer method. Jun 8, 2021
If a counter event source is disabled while in OnTimer, thread could end
up in infinite loop. This happens since _pollingIntervalInMilliseconds
is set to 0 when disabled, meaning that we could fail to advance
_nextPollingTimeStamp past now.

Fix makes sure it always gets advance event, even if _pollingIntervalInMilliseconds to 0.
@lateralusX lateralusX force-pushed the lateralusX/fix-infinite-loop-in-counter-group branch from 4fdd86c to 88ca1ae Compare June 9, 2021 08:19
@lateralusX
Copy link
Member Author

lateralusX commented Jun 9, 2021

Looks like this fix was investigated and done in parallel with #53836, it offers solution to same issue, so this could either be closed in favour of that implementation or we could use this implementation instead of for loop as proposed in #53836.

@lateralusX
Copy link
Member Author

Fixed and merged in #53836.

@lateralusX lateralusX closed this Jun 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant