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

Handle Counter Polling Interval of 0 #53836

Merged
merged 4 commits into from
Jun 10, 2021

Conversation

josalem
Copy link
Contributor

@josalem josalem commented Jun 7, 2021

Fixes #53564

  • default internal state to DateTime.MaxValue
  • re-check IsEnabled after not holding lock
  • add test for setting interval to 0

This will need to be backported to 3.1 and 5.0.

CC @tommcdon

* default internal state to DateTime.MaxValue
* re-check IsEnabled after not holding lock
* add test for setting interval to 0
@josalem josalem added this to the 6.0.0 milestone Jun 7, 2021
@josalem josalem requested review from noahfalk, davmason and a team June 7, 2021 20:52
@josalem josalem self-assigned this Jun 7, 2021
@ghost
Copy link

ghost commented Jun 7, 2021

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

Issue Details

Fixes #53564

  • default internal state to DateTime.MaxValue
  • re-check IsEnabled after not holding lock
  • add test for setting interval to 0

This will need to be backported to 3.1 and 5.0.

CC @tommcdon

Author: josalem
Assignees: josalem
Labels:

area-System.Diagnostics.Tracing

Milestone: 6.0.0

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping we can go even more targetted, suggestion inline : )

src/tests/tracing/eventcounter/gh53564.cs Show resolved Hide resolved
Copy link
Member

@davmason davmason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version LGTM

@lateralusX
Copy link
Member

Looks like I managed to repro/investigate/fix this issue when testing on iOS in parallel to this fix/investigation, #53887.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@josalem josalem merged commit 207bb77 into dotnet:main Jun 10, 2021
@josalem josalem deleted the dev/josalem/counter-interval-zero branch June 10, 2021 22:21
@josalem
Copy link
Contributor Author

josalem commented Jun 10, 2021

\backport to release/5.0

@hoyosjs
Copy link
Member

hoyosjs commented Jun 10, 2021

/backport to release/5.0

@github-actions
Copy link
Contributor

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/926712176

josalem pushed a commit to josalem/coreclr that referenced this pull request Jun 10, 2021
jeffschwMSFT pushed a commit to dotnet/coreclr that referenced this pull request Jun 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 10, 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.

EventCounter publishing thread can hang
6 participants