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

Improve waiting for metrics #4091

Merged
merged 3 commits into from
Jun 21, 2023
Merged

Improve waiting for metrics #4091

merged 3 commits into from
Jun 21, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jun 20, 2023 •
edited
Loading

Builds on top of #4087. I thought making these changes myself would be easier than explaining them in code review.

  • Add cancellation token to WaitForMeasurementsAsync.
  • Simplify timeout method to pass cancellation token to WaitForMeasurementsAsync.
  • Previously, waiters weren't removed from the collector if they timed out/were canceled. They are now removed.
  • The TCS is now created with RunContinuationsAsynchronously to ensure awaiting user code isn't run when holding the _measurements lock.

cc @geeknoid @noahfalk @tarekgh

Microsoft Reviewers: Open in CodeFlow

@JamesNK JamesNK requested a review from geeknoid June 20, 2023 02:19
@ghost ghost assigned JamesNK Jun 20, 2023
JamesNK
JamesNK commented Jun 20, 2023
View reviewed changes
src/Libraries/Microsoft.Extensions.Telemetry.Testing/Metering/MetricCollector.cs
@@ -136,8 +137,9 @@ public void Dispose()
// wake up anybody still waiting and inform them of the bad news: their horse is dead...
foreach (var w in _waiters)
{
w.TaskSource.SetException(MakeObjectDisposedException());
_ = w.TaskSource.TrySetException(MakeObjectDisposedException());
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if ObjectDisposedException is the right thing to do here. Setting the TCS to canceled might be more idiomatic. I'd need to look at what other APIs do for pending async tasks on dispose.

Copy link
Member

Choose a reason for hiding this comment

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

I decided not to use cancel semantics here since I think that would be surprising. The caller didn't cancel anything, they disposed an object.

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked up what another type does here: HttpClient cancels any outstanding requests instead of throwing ODE - https://github.com/dotnet/runtime/blob/05e25ff8c3a623df74720f430c270eba0f1d41b0/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs#L706-L710

I don't care much, but I'm pretty sure most .NET APIs clean up pending tasks on dispose by canceling the tasks, not throwing ODE.

@JamesNK JamesNK mentioned this pull request Jun 20, 2023
Merged
@geeknoid
Copy link
Member

So what's the scenario where a cancellation token is useful?

@JamesNK
Copy link
Member Author

JamesNK commented Jun 20, 2023

So what's the scenario where a cancellation token is useful?

The developer doesn't want to wait for measurements anymore for whatever reason. Maybe they have a test that waits for results from two collectors. When one successfully returns a result they cancel waiting for the other.

@geeknoid geeknoid force-pushed the geeknoid/mc branch 4 times, most recently from 4a41c34 to 2feb2f3 Compare June 20, 2023 12:59
Base automatically changed from geeknoid/mc to main June 20, 2023 14:21
@JamesNK
Copy link
Member Author

JamesNK commented Jun 21, 2023

@geeknoid I've rebased on main and resolved conflicts. All outstanding feedback has been addressed. Ready for review and merge?

@JamesNK JamesNK enabled auto-merge (squash) June 21, 2023 03:17
@JamesNK JamesNK merged commit 5441690 into main Jun 21, 2023
@JamesNK JamesNK deleted the jamesnk/await-refactor branch June 21, 2023 04:10
@ghost ghost added this to the 8.0 Preview7 milestone Jun 21, 2023
halter73 added a commit to dotnet/aspnetcore that referenced this pull request Jun 27, 2023
@halter73
React to dotnet/extensions#4091
e20a85f
@halter73 halter73 mentioned this pull request Jun 27, 2023
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@geeknoid geeknoid geeknoid approved these changes

Assignees

@JamesNK JamesNK

Labels
None yet
Projects
None yet
Milestone
8.0 Preview7
Development

Successfully merging this pull request may close these issues.
2 participants
@JamesNK @geeknoid