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

In Dns.RunAsync calls AfterResolution when canceled #104435

Merged

Conversation

rokonec
Copy link
Member

@rokonec rokonec commented Jul 4, 2024

Fixes: #92045

Context

When Dns.RunAsync is canceled before it runs its action, pairing Log.AfterResolution was not called causing permanently incorrect telemetry counters value. See description of #92045

Changes

Added AfterResolution in OnlyOnCanceled registered continuation.

@rokonec rokonec changed the title Calling AfterResolution when canceled In Dns.RunAsync calls AfterResolution when canceled Jul 4, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@rokonec rokonec requested a review from a team July 4, 2024 14:59
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

What happens if the CancellationToken is fired during the invocation of the delegate passed to prevTask.ContinueWith() (not the previously queued task)? Isn't it possible that both continuation delegates will be invoked leading to double call into AfterResolution?

Also, can we attempt to design a test for this? Idea for a "mini stress test": have a bunch parallel calls in a separate RemoteExecutor process, cancel some of them, count the ResolutionStart & ResolutionStop events.

@rokonec
Copy link
Member Author

rokonec commented Jul 8, 2024

What happens if the CancellationToken is fired during the invocation of the delegate passed to prevTask.ContinueWith() (not the previously queued task)? Isn't it possible that both continuation delegates will be invoked leading to double call into AfterResolution?

Because we dont pass CT to func() prevTask.ContinueWith() if the CancellationToken is fired during the invocation of the delegate the delegate will complete and call AfterRFesolution, AND task.ContinueWith for TaskContinuationOptions.OnlyOnCanceled will not be fired as it does not care about original task CT but only about completed task status - which will not be Cancelled.

TLDR: I believe it is OK

Also, can we attempt to design a test for this? Idea for a "mini stress test": have a bunch parallel calls in a separate RemoteExecutor process, cancel some of them, count the ResolutionStart & ResolutionStop events.

Yes it will be nice to have test to validate balanced ResolutionStart & ResolutionStop, however due to various caching on DNS I have no idea how to validate that failure case when resolution is canceled after it was added into task Continuation and before this continuation has been scheduled/run. I have tried it locally and failed to came up with stable solution. If cancelation toke is canceled before call of GetHostAddressesAsync, RunAsync is not called at all, and neither After or Before resolution can possibly be called. I don't see how to overcome this in RemoteExecutor.

If this is a high concert I would recommend to handle it in isolated PR, as this requires, IMO, some refactoring and investigations.

@rokonec rokonec merged commit 5bf04bd into dotnet:main Jul 10, 2024
79 of 83 checks passed
matouskozak added a commit to matouskozak/runtime that referenced this pull request Jul 11, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2024
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.

System.Net.Dns.RunAsync() does not decrement telemetry for canceled requests
4 participants