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.

Sign up for GitHub

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

Jump to bottom

Update IncrementingPollingCounter only on Timer thread #105548

Merged
merged 3 commits into from
Aug 6, 2024
Merged

Update IncrementingPollingCounter only on Timer thread #105548

merged 3 commits into from
Aug 6, 2024

Conversation

eterekhin
Copy link
Contributor

@eterekhin eterekhin commented Jul 26, 2024 •
edited
Loading

My idea is to call _totalValueProvider() only on Timer thread . It should fix deadlocks like #93175 since the Timer thread doesn't hold EventListener.EventListenersLock

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 26, 2024
This was referenced Jul 26, 2024
Open
Open
This was referenced Jul 27, 2024
Closed
Closed
Closed
@eterekhin eterekhin force-pushed the reset-counters-deadlock branch 2 times, most recently from e3066f0 to 0837ec4 Compare July 27, 2024 16:37
@eterekhin
Copy link
Contributor Author

@dotnet-policy-service agree

This was referenced Jul 28, 2024
Open
Open
This was referenced Jul 28, 2024
Open
Open
Open
@eterekhin eterekhin marked this pull request as ready for review July 29, 2024 06:18
@eterekhin eterekhin changed the title Reset counters within Timer thread Update IncrementingPollingCounter only on Timer thread Jul 29, 2024
@eterekhin
Copy link
Contributor Author

@davmason, Hey! May I ask you too look at this?

@davmason davmason requested a review from a team July 30, 2024 05:42
@davmason davmason added this to the 10.0.0 milestone Jul 30, 2024
@davmason
Copy link
Member

Added other people from dotnet-diag for review

This was referenced Aug 1, 2024
Open
Closed
noahfalk
noahfalk reviewed Aug 2, 2024
View reviewed changes
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.

This looks good to me - I suggested just a few minor tweaks.

src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/CounterGroup.cs Outdated Show resolved Hide resolved
noahfalk
noahfalk approved these changes Aug 5, 2024
View reviewed changes
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, thanks for your work on this @eterekhin!

@noahfalk
Copy link
Member

noahfalk commented Aug 5, 2024

@davmason - did you want to take another look?
@eterekhin - I assume you are ready for this to merge if @davmason is also happy with it?

davmason
davmason approved these changes Aug 6, 2024
View reviewed changes
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.

LGTM

@eterekhin
Copy link
Contributor Author

Thanks a lot for helping with this fix, folks!
@davmason, @noahfalk Could you please merge it? I guess the "Merge" button is hidden for me
image

@noahfalk noahfalk merged commit c94cacb into dotnet:main Aug 6, 2024
147 checks passed
@eterekhin eterekhin deleted the reset-counters-deadlock branch August 6, 2024 12:10
@noahfalk noahfalk mentioned this pull request Aug 11, 2024
Open
@tarekgh
Copy link
Member

tarekgh commented Aug 12, 2024

@noahfalk do we need a breaking change doc for this? if so, please file it through https://github.com/dotnet/docs/issues/new?assignees=gewarren&labels=breaking-change%2CPri1%2Cdoc-idea&projects=&template=02-breaking-change.yml&title=%5BBreaking+change%5D%3A+ and mark this PR with breaking change label?

@noahfalk
Copy link
Member

Would we typically write breaking change docs when we change an implementation choice that wasn't part of the original contract? I don't think of this as a breaking change in the formal sense but its true that some people may have taken dependencies on the threading behavior anyways.

@tarekgh
Copy link
Member

tarekgh commented Aug 12, 2024

We usually document any change that can cause breaking to the user. If this change is possible to break any behavior, I suggest documenting it, but it is your call if you think otherwise.

@noahfalk
Copy link
Member

Cool, I'll file it.

@MihaZupan MihaZupan mentioned this pull request Aug 12, 2024
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 12, 2024
This was referenced Sep 23, 2024
Closed
Closed
Closed
@noahfalk
Copy link
Member

noahfalk commented Oct 7, 2024

/backport to release/8.0

@github-actions github-actions bot unlocked this conversation Oct 7, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 7, 2024

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/11224036440

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2024
@github-actions github-actions bot mentioned this pull request Oct 7, 2024
Closed
4 tasks
@noahfalk
Copy link
Member

noahfalk commented Oct 7, 2024

/backport to release/8.0-staging

@github-actions github-actions bot unlocked this conversation Oct 7, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 7, 2024

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/11224091393

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davmason davmason davmason approved these changes

@noahfalk noahfalk noahfalk approved these changes

Assignees

@eterekhin eterekhin

Labels
area-System.Diagnostics.Tracing community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@eterekhin @davmason @noahfalk @tarekgh