-
Notifications
You must be signed in to change notification settings - Fork 772
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
Improve waiting for metrics #4091
Conversation
@@ -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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/Libraries/Microsoft.Extensions.Telemetry.Testing/Metering/MetricCollector.cs
Outdated
Show resolved
Hide resolved
So what's the scenario where a cancellation token is useful? |
src/Libraries/Microsoft.Extensions.Telemetry.Testing/Metering/MetricCollector.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry.Testing/Metering/MetricCollector.cs
Show resolved
Hide resolved
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. |
4a41c34
to
2feb2f3
Compare
0a126fa
to
0bd42b0
Compare
0bd42b0
to
100c83b
Compare
@geeknoid I've rebased on main and resolved conflicts. All outstanding feedback has been addressed. Ready for review and merge? |
Builds on top of #4087. I thought making these changes myself would be easier than explaining them in code review.
WaitForMeasurementsAsync
.WaitForMeasurementsAsync
.RunContinuationsAsynchronously
to ensure awaiting user code isn't run when holding the_measurements
lock.cc @geeknoid @noahfalk @tarekgh
Microsoft Reviewers: Open in CodeFlow